Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: separate background by preview/rendering #1027

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mxsdev
Copy link

@mxsdev mxsdev commented Apr 7, 2024

Closes: #1018

Some issues:

  • Currently the default setting for background will apply to both preview and rendering video settings - we could also introduce two separate settings (maybe best in a separate PR?)
  • Backwards Compatibility: Background settings from existing projects will be dropped and set to null for both preview and render
  • Forwards Compatibility: Background settings from newer projects read on older versions will not be recognized, and this will become null here as well
  • I did not write any tests for this change, but happy to (the code I changed didn't appear to be covered originally)

To address backwards compatibility issues, one approach is to introduce some sort of a "deprecated"/"hidden" flag for old fields to hide them in the preview, and then set the preview & render backgrounds accordingly on initial project load. Happy to introduce such a thing in a separate PR before merging this.

To address forwards compatibility issues, we could sync the deprecated background field above to one of the preview or render settings (not sure which is better). Although I'm not sure how concerned we are with forwards compatibility.

Let me know your thoughts @aarthificial

@aarthificial
Copy link
Contributor

one approach is to introduce some sort of a "deprecated"/"hidden" flag for old fields to hide them in the preview

There's already a disable method that can be called when defining meta fields that will hide them in the inspector.
But I don't think this would be necessary, undeclared fields in meta files are preserved so the migration can be performed by overriding the set method of ProjectMetadata:

export class ProjectMetadata /* ... */ {
  // ...

  public override set(
    value: Partial<ValueOf<ReturnType<typeof createProjectMetadata>>>,
  ) {
    if (value.shared && 'background' in value.shared) {
      const background = value.shared.background as Color | null;
      delete value.shared.background;
      if (value.preview) {
        value.preview.background = background;
      }
      if (value.rendering) {
        value.rendering.background = background;
      }
    }
    super.set(value);
  }
}

We also don't support forward compatibility since that's practically impossible. There are lots of new features being added in each version so trying to open a project with an older editor will almost always fail.

@@ -17,7 +17,7 @@ export function StageView({
...rest
}: StageViewProps) {
const localRef = useRef<HTMLDivElement>();
const {background} = useSharedSettings();
const {background} = usePreviewSettings();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StageView is used to display the animation in all three places:

  • PresentationMode - uses rendering settings
  • RenderingViewport - uses rendering settings
  • PreviewStage - uses preview settings

So the background should probably be passed here as a prop depending on the context in which the stage is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preview-only Background Color
2 participants