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

Fixes Bounty #Generate equirectangular 360 panorama from babylon.js scene #14251

Merged
merged 11 commits into from Sep 4, 2023
Merged

Fixes Bounty #Generate equirectangular 360 panorama from babylon.js scene #14251

merged 11 commits into from Sep 4, 2023

Conversation

lokiiarora
Copy link
Contributor

Hey, I'm a first-time contributor to Babylon.js, love the community, and would love it if someone could review this PR.
This PR is based on @Pryme8 's work on this playground here.

@bjsplat
Copy link
Collaborator

bjsplat commented Sep 3, 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 Sep 3, 2023

@RaananW
Copy link
Member

RaananW commented Sep 4, 2023

That's really great! thank you so much for that :-)

Got a question. I tried it, and it works great. The only thing I noticed - the image generated is flipped:

https://playground.babylonjs.com/?snapshot=refs/pull/14251/merge#Z6SWJU#5

results in:

equirectangular_capture (1)

Was that intentional?

@lokiiarora
Copy link
Contributor Author

That's really great! thank you so much for that :-)

Got a question. I tried it, and it works great. The only thing I noticed - the image generated is flipped:

https://playground.babylonjs.com/?snapshot=refs/pull/14251/merge#Z6SWJU#5

results in:

equirectangular_capture (1)

Was that intentional?

Ouch, this was not intentional, I guess I'm not passing invertY flag, let me just do it

@lokiiarora
Copy link
Contributor Author

@RaananW fixed on local, will verify on remote :)

@lokiiarora lokiiarora marked this pull request as ready for review September 4, 2023 12:55
@lokiiarora
Copy link
Contributor Author

lokiiarora commented Sep 4, 2023

looks fine now @RaananW
equirectangular_capture (7)

Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

LGTM! just a note about the shader code itself. I'll also wait for others to comment, but I will approve right after the shader code moves.

packages/dev/core/src/Misc/equirectangularCapture.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@carolhmj carolhmj left a comment

Choose a reason for hiding this comment

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

LGTM 😸

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.

Aside of naming, this looks great to me !!!

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.

Could you allow an option to pass the probe in ?

@lokiiarora
Copy link
Contributor Author

Could you allow an option to pass the probe in ?

has been added, do check if this is how you would want it to be?

@RaananW RaananW merged commit 038974f into BabylonJS:master Sep 4, 2023
10 checks passed
@sebavan
Copy link
Member

sebavan commented Sep 4, 2023

Nice, looks great !!!!

@evidanary
Copy link

@RaananW and @lokiiarora looks like the feature does not work for post processing. We've tested it for a scene that uses the DefaultRenderingPipeline with tonemapping enabled. I've tested it on my end and can confirm the tone mapping does not get applied when generating the equirectangular image. Is this a limitation/bug of this feature or does the underlying reflectionprobes support post-processing?

@sebavan
Copy link
Member

sebavan commented Jan 4, 2024

I do not think it is supported at the moment. It would be great if you could create a topic in the forum with a repro in the playground. This way we could simply check how hard this could be to support.

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

7 participants