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.

Sign up for GitHub

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

Jump to bottom

feat: support formats option for multirendertarget #13678

Merged
merged 6 commits into from
Apr 5, 2023
Merged

feat: support formats option for multirendertarget #13678

merged 6 commits into from
Apr 5, 2023

Conversation

newbeea
Copy link
Contributor

@newbeea newbeea commented Mar 28, 2023 •
edited
Loading

Add support for setting formats option of multirendertarget

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 28, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

1 similar comment
@bjsplat
Copy link
Collaborator

bjsplat commented Mar 28, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 28, 2023

Snapshot stored with reference name:
refs/pull/13678/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13678/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13678/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/13678/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/13678/merge
https://gui.babylonjs.com/?snapshot=refs/pull/13678/merge
https://nme.babylonjs.com/?snapshot=refs/pull/13678/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/13678/merge#BCU1XR#0

@newbeea newbeea marked this pull request as draft March 28, 2023 07:48
@newbeea newbeea marked this pull request as ready for review March 28, 2023 07:49
@newbeea newbeea closed this Mar 28, 2023
@newbeea newbeea reopened this Mar 28, 2023
@Popov72
Copy link
Contributor

Popov72 commented Mar 28, 2023

Thanks, we planned to add this feature in the coming weeks!

However, we need to wait for #13435 to be merged, because it changes a number of things in this area (notably in Engines/Extensions/engine.multiRender.ts).

So, can you wait for #13435 to be merged and update your PR with it? Switching this PR to Draft in the meantime.

Thanks a lot!

@Popov72 Popov72 marked this pull request as draft March 28, 2023 11:01
@newbeea newbeea force-pushed the set-multirendertarget-format branch from da645eb to d2bc9b2 Compare April 4, 2023 07:19
@newbeea newbeea force-pushed the set-multirendertarget-format branch from d2bc9b2 to 994ba87 Compare April 4, 2023 07:22
@newbeea newbeea marked this pull request as ready for review April 4, 2023 09:06
Popov72
Popov72 requested changes Apr 4, 2023
View reviewed changes
Copy link
Contributor

@Popov72 Popov72 left a comment

Choose a reason for hiding this comment

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

There's still Engines/RenderTargetWrapper._cloneRenderTargetWrapper to update and we are good to go!

packages/dev/core/src/Materials/Textures/multiRenderTarget.ts Show resolved Hide resolved
@sebavan sebavan requested a review from Popov72 April 4, 2023 16:25
sebavan
sebavan reviewed Apr 4, 2023
View reviewed changes
Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

LGTM, I ll merge after a final @Popov72 review

@sebavan sebavan enabled auto-merge April 4, 2023 16:29
@Popov72
Copy link
Contributor

Popov72 commented Apr 4, 2023

There's:

There's still Engines/RenderTargetWrapper._cloneRenderTargetWrapper to update and we are good to go!

And it will be ok!

auto-merge was automatically disabled April 5, 2023 06:07

Head branch was pushed to by a user without write access

@newbeea
Copy link
Contributor Author

newbeea commented Apr 5, 2023 •
edited
Loading

There's:

There's still Engines/RenderTargetWrapper._cloneRenderTargetWrapper to update and we are good to go!

And it will be ok!

Sorry I didn't notice that, I have pushed that now.

@sebavan
Copy link
Member

sebavan commented Apr 5, 2023

Looks perfect to me, pinging @Popov72 just to be sure :-)

@sebavan sebavan merged commit 45503bf into BabylonJS:master Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebavan sebavan sebavan left review comments

@Popov72 Popov72 Popov72 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@newbeea @bjsplat @Popov72 @sebavan