Fix view z calculation for 'distance' used as sort key

[superdump]

Objective

• Fix incorrect mesh sorting in wireframe and examples that use custom sorts

Solution

• The model translation must be transformed by the inverse view transform. If you consider an object directly in front of you and you turn your head to the left you're now looking to the left of the object. The way rendering works is that you instead consider that your head remains stationary, and you rotate everything else in the opposite direction to how you want to turn your head. So, you transform the model by the inverse view transform.
• To get the view space z of the object, we can calculate the dot product of row 2 of the inverse view matrix and the object's translation, which is stored in column 3 of the object transform.
• Add constructors for Opaque3d, AlphaMask3d, Transparent3d, Transparent2d in order to correctly calculate the sort key using the inverse view transform and mesh transform
• Make phase item struct members private to encourage use of the constructors

---

Changelog

• Fixed: Sort order of wireframe meshes
• Changed: Add constructors for Opaque3d, AlphaMask3d, Transparent3d, Transparent2d in order to correctly calculate the sort key using the inverse view transform and mesh transform, and make phase item struct members private to encourage use of the constructors

[alice-i-cecile]

The math makes sense, and the comments are helpful. I wish users didn't have to reason about whether they needed the view matrix or the inverse view matrix, but I don't immediately see a way around it at the level of abstraction we're using in the shader examples.

[alice-i-cecile]

I am not thrilled that users have to reason about this directly, but I'm not immediately sure if there's a clean abstraction to expose.

[superdump]

Yeah, I was trying to think of a clear and efficient way of doing this. Calculating a matrix inverse is expensive. The matrices are column-major so extracting the row has a cost too. I don't know if there is much value to adding a function to calculate the inverse view matrix row 2. And then another for calculating the dot product of that with the translation/position of interest.

I suppose extracting row 2 isn't so expensive, so providing a couple of methods for calculating the view z from the inverse view and model transforms, and from the inverse view and a translation/point could be helpful. I don't know how we can make them discoverable though. Maybe we could solve it by making inlined constructor functions... something like:

impl Transparent3d {
    #[inline]
    pub fn from_mesh_transform(
        entity: Entity,
        pipeline: CachedRenderPipelineId,
        draw_function: DrawFunctionId,
        inverse_view: &Mat4,
        mesh_transform: &Mat4,
    ) -> Self {
        Self::from_mesh_translation(
            entity,
            pipeline,
            draw_function,
            inverse_view,
            mesh_transform.col(3),
        )
    }

    #[inline]
    pub fn from_mesh_translation(
        entity: Entity,
        pipeline: CachedRenderPipelineId,
        draw_function: DrawFunctionId,
        inverse_view: &Mat4,
        mesh_translation: Vec4,
    ) -> Self {
        Self {
            distance: inverse_view.row(2).dot(mesh_translation),
            pipeline,
            entity,
            draw_function,
        }
    }
}

[alice-i-cecile]

I like the constructor functions; it gives us a place to put docs, and avoids having users maintain this subtle logic on their own.

[superdump]

I realised the only place I had seen the Vec4 variant was for lights and that is internal. Everything else used the full transform. Check it out! :)

Also, I did a quick test to verify that this didn't have any impact on performance of adding Transparent2d phase items to the Transparent2d RenderPhase for sprites due to using a constructor function. I added #[inline] annotations but wanted to be sure. many_sprites queue_sprites takes 11.39ms on main and 11.47ms on this PR. 0.7% slower. Probably just noise...

[alice-i-cecile]

Much better! I really like the addition of the builder methods: they make this API much more self-documenting and discoverable, in addition to protecting users from this mistake in the future.

[jakobhellermann]

Looks good to me.

[jakobhellermann]

Would there ever be a use case for creating an Opaque3d without first calculating the matrices? It looks like from_mesh_transform is the only way to construct the phase item now, which may be restricting?

Could be solved by either making the field public again or adding another unopinionated constructor if this is even useful.

[superdump]

Hmm, I did it this way to prevent people from doing the wrong thing. You could pass in zero or identity matrices or something I guess.

[superdump]

@cart I'd like your input on the phase item constructors that I've added to try to force correct calculation. It feels a bit controversial to me. Also, I'm not super keen on having to pass matrices as the API but the main alternative I see is to try to get people to pass inverse view row 2 and model column 3 by naming the function parameters as such and that feels sketchy too.

[cart]

I definitely like these constructors as they simplify common use cases / cut down on sources of error. But I'm opposed to making them the only way to construct phase items. These phases are intended to be as generic and unopinionated as possible. Ex: we shouldn't be dictating that Opaque phase items have a Mat4 transform (or use an inverse_view when computing distance).

[superdump]

I definitely like these constructors as they simplify common use cases / cut down on sources of error. But I'm opposed to making them the only way to construct phase items. These phases are intended to be as generic and unopinionated as possible. Ex: we shouldn't be dictating that Opaque phase items have a Mat4 transform (or use an inverse_view when computing distance).

Cool. I think that probably frees me up to use a slightly less ergonomic but better-performing and documented constructor then that allows caching the row 2 vector of the inverse view matrix rather than this approach that requires extracting and constructing it for each call. I just did it this way to try to force doing the right thing because people had done the wrong thing and then copied and pasted it to other places and it spread. :/ As suck it felt like it needed protection against doing the wrong thing. That said, someone could just pass the view matrix instead of inverse view and then it’s the same problem as originally. Not really sure how to encourage or enforce correct usage.

[cart]

Not really sure how to encourage or enforce correct usage.

I think this is a "with great power comes great responsibility" situation. Generally people touching these apis should already be comfortable with the render pipeline and its requirements. I'm ok with some level of trust here, in the interest of flexibility / clarity. Ex: i dont think moving the concept of InverseView(Mat4) into the type system is worth the cognitive load of the abstraction.

[superdump]

Not really sure how to encourage or enforce correct usage.

I think this is a "with great power comes great responsibility" situation. Generally people touching these apis should already be comfortable with the render pipeline and its requirements. I'm ok with some level of trust here, in the interest of flexibility / clarity. Ex: i dont think moving the concept of InverseView(Mat4) into the type system is worth the cognitive load of the abstraction.

Then it seems we will just have to expect that people will do it wrong if they aren’t familiar with how matrix transformations work and why the inverse view matrix is needed in this case. Alternatively put, we expect that people to understand matrix transformations and that the inverse view matrix is needed here. Ok.

I’ll change the constructors to use the vectors instead, for better performance, and double check the documentation is present and clear for those constructors, and make the struct members public again.

[superdump]

I think #5014 is a better solution to this but I’ll see how it pans out.

[superdump]

Fixed by #5014