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

Deprecated ForwardRenderer.renderComposition to keep compatibility. #4292

Merged
merged 1 commit into from
Jun 1, 2022

Conversation

mvaligursky
Copy link
Contributor

.. to keep application using this functioning.

@mvaligursky mvaligursky self-assigned this Jun 1, 2022
@willeastcott
Copy link
Contributor

From an architectural point of view, AppBase feels like a strange place to find a function like renderComposition. However, I might expect an app to own a scene and a renderer, where the scene can be rendered with the renderer. Thoughts?

@mvaligursky
Copy link
Contributor Author

From an architectural point of view, AppBase feels like a strange place to find a function like renderComposition. However, I might expect an app to own a scene and a renderer, where the scene can be rendered with the renderer. Thoughts?

I agree with you, but the Scene is not currently usable as a container to render, as we only can have a single scene per application. At the moment, the RenderComposition is set up as the thing you can have multiple copies of (even this does not really work, and there are issues with it), so the current API matches this limitation. Longer term, we're hoping to allow to have multiple scenes and render those as needed.
That's one of the reasons APpBase.renderComposition is not public as well .. even though people for a lack of better way sometimes use it.

@willeastcott
Copy link
Contributor

If it's not public, then fine - it'd be good to refactor this in future. Approving...

@mvaligursky mvaligursky merged commit 5487782 into main Jun 1, 2022
@mvaligursky mvaligursky deleted the mvaligursky-forward-renderer-render branch June 1, 2022 10:03
slimbuck pushed a commit that referenced this pull request Jun 1, 2022
…4292)

Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
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.

None yet

3 participants