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

Rename several transform built-ins in shaders #59092

Closed
wants to merge 0 commits into from
Closed

Rename several transform built-ins in shaders #59092

wants to merge 0 commits into from

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Mar 13, 2022

Proposed renames from #54161 (comment):

Shader builtin matrices rename suggestions

Spatial Shaders

  • WORLD_MATRIX rename to MODEL_TO_WORLD
  • WORLD_NORMAL_MATRIX rename to MODEL_TO_WORLD_NORMAL
  • MODELVIEW_MATRIX rename to MODEL_TO_VIEW
  • MODELVIEW_NORMAL_MATRIX rename to MODEL_TO_VIEW_NORMAL
  • CAMERA_MATRIX rename to VIEW_TO_WORLD
  • INV_CAMERA_MATRIX rename to WORLD_TO_VIEW
  • PROJECTION_MATRIX rename to VIEW_TO_CLIP
  • INV_PROJECTION_MATRIX rename to CLIP_TO_VIEW

CanvasItem Shaders

  • WORLD_MATRIX rename to MODEL_TO_WORLD
  • CANVAS_MATRIX rename to WORLD_TO_CANVAS
  • SCREEN_MATRIX rename to CANVAS_TO_SCREEN

@Chaosus Chaosus requested review from a team as code owners March 13, 2022 08:31
@Chaosus Chaosus added this to the 4.0 milestone Mar 13, 2022
@Chaosus Chaosus force-pushed the shader_renames branch 2 times, most recently from a7c8e2a to db6971b Compare March 13, 2022 08:40
@Chaosus Chaosus requested a review from clayjohn March 13, 2022 08:45
@clayjohn
Copy link
Member

These were discussed in the rendering chat when I first proposed the changes and it appeared then that most rendering contributors were opposed to changing the names. It should likely now be re-discussed with your PR to see what people think.

@clayjohn
Copy link
Member

We discussed this in the PR review meeting today and we agreed not to rename all of the matrices, but we will rename a few to match more common conventions

WORLD_MATRIX rename to MODEL_MATRIX
WORLD_NORMAL_MATRIX rename to MODEL_NORMAL_MATRIX

CAMERA_MATRIX rename to INV_VIEW_MATRIX
INV_CAMERA_MATRIX rename to VIEW_MATRIX

@Chaosus Chaosus requested a review from a team as a code owner March 18, 2022 08:29
@Chaosus Chaosus force-pushed the shader_renames branch 3 times, most recently from a85ee73 to fd285aa Compare March 18, 2022 08:40
@Chaosus Chaosus closed this Mar 18, 2022
@Chaosus Chaosus deleted the shader_renames branch March 18, 2022 08:55
@Chaosus
Copy link
Member Author

Chaosus commented Mar 18, 2022

Damn, I think I'm suddenly deleted it.

@akien-mga
Copy link
Member

akien-mga commented Mar 18, 2022 •
edited
Loading

You can likely find the code back with git reflog. Otherwise it's in fd285aa (but I don't manage to make a patch from it)

@Chaosus Chaosus restored the shader_renames branch March 18, 2022 09:06
@Chaosus Chaosus mentioned this pull request Mar 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clayjohn clayjohn Awaiting requested review from clayjohn

Assignees
No one assigned
Labels
archived breaks compat enhancement topic:shaders
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Chaosus @clayjohn @akien-mga