Normalise matrix naming #13489

ricky26 · 2024-05-23T22:31:25Z

Objective

Solution

Name all matrices x_from_y, for example world_from_view.

Testing

I've tested most of the 3D examples. The lighting example particularly should hit a lot of the changes and appears to run fine.

Changelog

Renamed matrices across the engine to follow a y_from_x naming, making the space conversion more obvious.

Migration Guide

Frustum's from_view_projection, from_view_projection_custom_far and from_view_projection_no_far were renamed to from_clip_from_world, from_clip_from_world_custom_far and from_clip_from_world_no_far.
ComputedCameraValues::projection_matrix was renamed to clip_from_view.
CameraProjection::get_projection_matrix was renamed to get_clip_from_view (this affects implementations on Projection, PerspectiveProjection and OrthographicProjection).
ViewRangefinder3d::from_view_matrix was renamed to from_world_from_view.
PreviousViewData's members were renamed to view_from_world and clip_from_world.
ExtractedView's projection, transform and view_projection were renamed to clip_from_view, world_from_view and clip_from_world.
ViewUniform's view_proj, unjittered_view_proj, inverse_view_proj, view, inverse_view, projection and inverse_projection were renamed to clip_from_world, unjittered_clip_from_world, world_from_clip, world_from_view, view_from_world, clip_from_view and view_from_clip.
GpuDirectionalCascade::view_projection was renamed to clip_from_world.
MeshTransforms' transform and previous_transform were renamed to world_from_local and previous_world_from_local.
MeshUniform's transform, previous_transform, inverse_transpose_model_a and inverse_transpose_model_b were renamed to world_from_local, previous_world_from_local, local_from_world_transpose_a and local_from_world_transpose_b (the Mesh type in WGSL mirrors this, however transform and previous_transform were named model and previous_model).
Mesh2dTransforms::transform was renamed to world_from_local.
Mesh2dUniform's transform, inverse_transpose_model_a and inverse_transpose_model_b were renamed to world_from_local, local_from_world_transpose_a and local_from_world_transpose_b (the Mesh2d type in WGSL mirrors this).
In WGSL, in bevy_pbr::mesh_functions, get_model_matrix and get_previous_model_matrix were renamed to get_world_from_local and get_previous_world_from_local.
In WGSL, bevy_sprite::mesh2d_functions::get_model_matrix was renamed to get_world_from_local.

assets/shaders/irradiance_volume_voxel_visualization.wgsl

crates/bevy_pbr/src/render/light.rs

crates/bevy_pbr/src/light/mod.rs

examples/3d/irradiance_volumes.rs

superdump

Mostly looks great, thanks for doing it. Left a couple of comments.

…e/matrix-naming

ricky26 · 2024-05-31T01:33:43Z

I've applied some review feedback, merged latest main back in and tested several examples (particularly lighting, which should test most of the changes).

IceSentry · 2024-06-02T03:39:55Z

@ricky26 can you fix the conflicts?

…e/matrix-naming

…e/matrix-naming # Conflicts: # crates/bevy_pbr/src/prepass/mod.rs

ricky26 · 2024-06-02T20:35:19Z

I've merged main in again after the skybox motion vectors and fixed up the introduced changes.

superdump

Assuming this is well tested, LGTM.

ricky26 · 2024-06-03T10:20:59Z

I think I've run enough examples to touch all of the changed code. I did catch some missed renames in the meshlet shaders.

I've finished filling in the migration notes with everything public that's been renamed.

IceSentry

I tested a whole bunch of examples and didn't find any regressions. Thank you for working on this!

Approving, assuming the conflicts are fixed of course.

alice-i-cecile · 2024-06-03T16:24:30Z

@ricky26 once merge conflicts are resolved holler at me and I'll merge this in.

…e/matrix-naming # Conflicts: # crates/bevy_pbr/src/light/mod.rs

ricky26 · 2024-06-03T16:51:40Z

@alice-i-cecile, I've merged in main and tested at least the lighting & meshlet examples. (I've been testing some others whilst CI is running too.) Should be good once CI finishes. I think the only conflict was the clustering move.

alice-i-cecile · 2024-06-03T20:51:59Z

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1331 if you'd like to help out.

[13289](bevyengine/bevy#13489) introduced matrix naming changes, including `view_proj` which becomes `clip_from_world`

As per the other changes in bevyengine#13489

As per the other changes in #13489 `view.inverse_clip_from_world` should be `world_from_clip`. # Objective fixes #13749 ## Solution Modified lines.wgsl to use the right name as the current name does not exist. ## Testing I ran the 3d_gizmos example and pressed "p". ![screenshot-2024-06-08-at-13 21 22@2x](https://github.com/bevyengine/bevy/assets/551247/b8bfd3db-8273-4606-9dae-040764339883) ![screenshot-2024-06-08-at-13 21 26@2x](https://github.com/bevyengine/bevy/assets/551247/2619f1ae-ce83-44d7-a9fc-07e686950887)

Addresses Bevy [13489](bevyengine/bevy#13489)

* Update to 0.14.0-rc.2 * [12997](bevyengine/bevy#12997): rename `multi-threaded` to `multi_threaded` * RenderAssets<Image> is now RenderAssets<GpuImage> Implemented in [12827](bevyengine/bevy#12827) * FloatOrd is now in bevy_math implemented in [12732](bevyengine/bevy#12732) * convert Transparent2d::dynamic_offset to extra_index [12889](bevyengine/bevy#12889) Gpu Frustum Culling removed the dynamic_offset of Transparent2d and it became `extra_index` with the special value `PhaseItemExtraIndex::NONE`, which indicates the `None` that was here previously * RenderPhase<Transparent2d> -> ViewSortedRenderPhases<Transparent2d> [12453](https://github.com/StarArawn/bevy_ecs_tilemap/pull/bevyengine/bevy#12453): Render phases are now binned or sorted. Following the changes in the `mesh2d_manual` [example](https://github.com/bevyengine/bevy/blob/ecdd1624f302c5f71aaed95b0984cbbecf8880b7/examples/2d/mesh2d_manual.rs#L357-L358): use the `ViewSortedRenderPhases` resource. * get_sub_app_mut is now an Option in [9202](https://github.com/StarArawn/bevy_ecs_tilemap/pull/bevyengine/bevy/pull/9202) SubApp access has changed * GpuImage::size f32 -> u32 via UVec2 [11698](bevyengine/bevy#11698) changed `GpuImage::size` to `UVec2`. Right above this, `Extent3d` does the same thing, so I'm taking a small leap and assuming can `as`. * GpuMesh::primitive_topology -> key_bits/BaseMeshPipeline [12791](bevyengine/bevy#12791) the `primitive_topology` field on `GpuMesh` was removed in favor of `key_bits` which can be constructed using `BaseMeshPipeline::from_primitive_topology` * RenderChunk2d::prepare requires &mut MeshVertexBufferLayouts now [12216](bevyengine/bevy#12216) introduced an argument `&mut MeshVertexBufferLayouts` to `get_mesh_vertex_buffer_layout`, which bevy_ecs_tilemap calls in `RenderChunk2d::prepare` * into_linear_f32 -> color.0.linear().to_f32_array(), [12163](bevyengine/bevy#12163) bevy_color was created and Color handling has changed. Specifically Color::as_linear_rgba_f32 has been removed. LinearRgba is now its own type that can be accessed via [`linear()`](https://docs.rs/bevy/0.14.0-rc.2/bevy/color/enum.Color.html#method.linear) and then converted. * Must specify type of VisibleEntities when accessing [12582](bevyengine/bevy#12582) divided `VisibleEntities` into separate lists. So now we have to specify which kind of entity we want. I think we want the Mesh here, and I think we can get rid of the `.index` calls on Entity since Entity [already compares bits](https://docs.rs/bevy_ecs/0.14.0-rc.2/src/bevy_ecs/entity/mod.rs.html#173) for optimized codegen purposes. Waiting to do that until the other changes are in though so as to not change functionality until post-upgrade. * app.world access is functions now - [9202](bevyengine/bevy#9202) changed world access to functions. [relevent line](https://github.com/bevyengine/bevy/pull/9202/files#diff-b2fba3a0c86e496085ce7f0e3f1de5960cb754c7d215ed0f087aa556e529f97fR640) - This also surfaced [12655](bevyengine/bevy#12655) which removed `Into<AssetId<T>>` for `Handle<T>`. using a reference or .id() is the solution here. * We don't need `World::cell`, and it doesn't exist anymore In [12551](bevyengine/bevy#12551) `WorldCell` was removed. ...but it turns out we don't need it or its replacement anyway. * examples error out unless this bevy bug is addressed with these features being added bevyengine/bevy#13728 * check_visibility is required for the entity that is renderable As a result of [12582](bevyengine/bevy#12582) `check_visibility` must be implemented for the "renderable" tilemap entities. Doing this is trivial by taking advantage of the existing `check_visibility` type arguments, which accept a [`QF: QueryFilter + 'static`](https://docs.rs/bevy/0.14.0-rc.2/bevy/render/view/fn.check_visibility.html). The same `QueryFilter`` is used when checking `VisibleEntities`. I've chosen `With<TilemapRenderSettings` because presumably if the entity doesn't have a `TilemapRenderSettings` then it will not be rendering, but this could be as sophisticated or simple as we want. For example `WithLight` is currently implemented as ```rust pub type WithLight = Or<(With<PointLight>, With<SpotLight>, With<DirectionalLight>)>; ``` * view.view_proj -> view.clip_from_world [13289](bevyengine/bevy#13489) introduced matrix naming changes, including `view_proj` which becomes `clip_from_world` * color changes to make tests runnable * clippy fix * Update Cargo.toml Co-authored-by: Rob Parrett <[email protected]> * Update Cargo.toml Co-authored-by: Rob Parrett <[email protected]> * final clippy fixes * Update Cargo.toml Co-authored-by: Rob Parrett <[email protected]> * Simplify async loading in ldtk/tiled helpers See Bevy #12550 * remove second allow lint * rc.3 bump * bump version for major release * remove unused features --------- Co-authored-by: Rob Parrett <[email protected]>

alice-i-cecile added this to the 0.14 milestone May 23, 2024

Try to normalise matrix naming

0e0ee30

ricky26 force-pushed the feature/matrix-naming branch from 7956006 to 0e0ee30 Compare May 23, 2024 22:35