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

Add support for Cube Map Rotation and expose Skybox Rotation #2527

Merged
merged 20 commits into from
Nov 10, 2020

Conversation

raytranuk
Copy link
Contributor

@raytranuk raytranuk commented Nov 4, 2020

Fixes #2505

Public API additions:

 * @class
 * @name pc.Scene
 *
 * @property {pc.Quat} skyboxRotation The rotation of the skybox to be displayed. Defaults to pc.Quat.IDENTITY

Example:

    var r = new pc.Quat();
    r.setFromEulerAngles(42, 42, 0);
    app.scene.skyboxRotation = r;

I confirm I have signed the Contributor License Agreement.

@raytranuk raytranuk added enhancement area: graphics Graphics related issue labels Nov 4, 2020
@raytranuk raytranuk requested a review from a team November 4, 2020 00:15
@raytranuk raytranuk self-assigned this Nov 4, 2020
@raytranuk raytranuk marked this pull request as ready for review November 4, 2020 00:27
…o all cubemap related shader chunks

It is very likely that this will cause some custom shader chunks to break
@raytranuk raytranuk changed the title Allow Environment Maps to Rotate Add support for Cube Map Rotation Nov 5, 2020
Copy link
Member

@slimbuck slimbuck left a comment

Choose a reason for hiding this comment

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

As a user, I think it's confusing that setting an identity rotation matrix results in a mirror transform. I'd much rather keep the -1x always and optionally multiply in the rotation matrix.

Also, is it possible to have this rotation logic (the -1x and optional rotation matrix) encapsulated into single chunk/function? This way we can modify/customise easily in future.

@slimbuck
Copy link
Member

slimbuck commented Nov 5, 2020

Alternatively you could apply the -1x to the matrix when it's being set on the shader.

@raytranuk
Copy link
Contributor Author

As a user, I think it's confusing that setting an identity rotation matrix results in a mirror transform. I'd much rather keep the -1x always and optionally multiply in the rotation matrix.

Also, is it possible to have this rotation logic (the -1x and optional rotation matrix) encapsulated into single chunk/function? This way we can modify/customise easily in future.

Alternatively you could apply the -1x to the matrix when it's being set on the shader.

I will move the rotation logic into a function and put it in a new chunk.

For the -1x flip, maybe the best way for users is to expose an extra boolean flag which defaults to true to determine is the -1x should be applied to the rotation - and maybe switch back to Quat from Mat3 for the rotation property?

@mvaligursky
Copy link
Contributor

For the -1x flip, maybe the best way for users is to expose an extra boolean flag which defaults to true to determine is the -1x should be applied to the rotation - and maybe switch back to Quat from Mat3 for the rotation property?

yep I agree with this. Ideally we don't convert quat->matrix each frame, so it would need to store additional Mat3 as well?

isRenderTarget flag added to pc.Texture
skyboxRotation is now pc.Quat again, but with a private pc.Mat3 that only gets updated when needed.
@willeastcott
Copy link
Contributor

Will this fully address #189?

@raytranuk
Copy link
Contributor Author

Just curious why the docs for each property have been made single line? We normally try to make an effort for JSDoc blocks to stick to a reasonable number of columns (the coding standards never specified this maximum though). But it's good to be able to read them without having to scroll horizontally.

I restored it to multi-line

src/scene/scene.js Outdated Show resolved Hide resolved
@raytranuk raytranuk requested a review from mvaligursky November 6, 2020 12:10
src/scene/scene.js Outdated Show resolved Hide resolved
…v-map-rotate

# Conflicts:
#	src/graphics/program-lib/chunks/reflectionPrefilteredCube.frag
@raytranuk raytranuk requested a review from mvaligursky November 9, 2020 18:48
src/scene/scene.js Outdated Show resolved Hide resolved
src/scene/scene.js Outdated Show resolved Hide resolved
src/scene/scene.js Outdated Show resolved Hide resolved
@Maksims
Copy link
Collaborator

Maksims commented Nov 10, 2020

PR name is misleading - this is not cubemap rotation, but skybox rotation.

Is it possible to rotate cubemap that is applied on material directly?
Box projection cubemaps been limited by not able to rotate them, leading to limited use cases. One of them is moving objects, e.g. train with box projected cubemap inside.

@raytranuk
Copy link
Contributor Author

PR name is misleading - this is not cubemap rotation, but skybox rotation.

Is it possible to rotate cubemap that is applied on material directly?
Box projection cubemaps been limited by not able to rotate them, leading to limited use cases. One of them is moving objects, e.g. train with box projected cubemap inside.

@Maksims - You are right, this PR now only exposes skybox rotation, even though the shader code does support any cubemap to be rotated. A subsequent PR will also expose non-skybox cube map rotation - and we are discussing the best place to store the rotation. I'll adjust the name of this PR.

@raytranuk raytranuk changed the title Add support for Cube Map Rotation Add support for Cube Map Rotation and expose Skybox Rotation Nov 10, 2020
@raytranuk raytranuk merged commit a7bbb5f into master Nov 10, 2020
@raytranuk raytranuk deleted the env-map-rotate branch November 10, 2020 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow env cube maps to be rotated
5 participants