Skip to content

Improve render sorting for alpha pass #106593

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Faolan-Rad
Copy link
Contributor

Made sorting more deterministic for transparent objects by making it fallow

Sorting Order

  1. material priority
  2. object depth
  3. stacked sorting offset for when depth is approximately the same
  4. surface index used because most other engines work in this way
  5. material depth used for next-pass sorting
  6. geometry id used for constant render sorting to make more predictable

Also added stacked sorting offset for when the depth is the same but a user wants to render multiple layers of objects but don't want it effecting neighboring objects

After fix
image
gizmos no longer mess with sort
image

Before fix inconsistent sorting and flashing
image
gizmos mess with sort
image

The project I made for testing
project.zip

@Faolan-Rad Faolan-Rad requested review from a team as code owners May 19, 2025 12:57
@Faolan-Rad
Copy link
Contributor Author

I don't know why the docs check failed for Linux / Editor w/ Mono given that it is giving me a completely incorrect sorting_stacked_offset that isn't an int so I wonder if the system is confusing sorting_offset and sorting_stacked_offset so I don't know what the problem mainly because
ADD_PROPERTY(PropertyInfo(Variant::INT, "sorting_stacked_offset"), "set_stacked_sorting_offset", "get_stacked_sorting_offset");
is set

@Calinou
Copy link
Member

Calinou commented May 19, 2025

I see this adds a new property that acts as a secondary sorting offset for objects whose depth is identical (which acts different to the existing one). Is there a proposal tracking it? This should be discussed in a proposal to make sure there's an established need for it.

I don't know why the docs check failed for Linux / Editor w/ Mono given that it is giving me a completely incorrect sorting_stacked_offset that isn't an int so I wonder if the system is confusing sorting_offset and sorting_stacked_offset so I don't know what the problem mainly because

You need to reorder the property in the generated class reference to follow alphabetical order. I suggest running --doctool locally and committing the result.

@Faolan-Rad
Copy link
Contributor Author

I see this adds a new property that acts as a secondary sorting offset for objects whose depth is identical (which acts different to the existing one). Is there a proposal tracking it? This should be discussed in a proposal to make sure there's an established need for it.

I don't know why the docs check failed for Linux / Editor w/ Mono given that it is giving me a completely incorrect sorting_stacked_offset that isn't an int so I wonder if the system is confusing sorting_offset and sorting_stacked_offset so I don't know what the problem mainly because

You need to reorder the property in the generated class reference to follow alphabetical order. I suggest running --doctool locally and committing the result.

I realized the property methods where set to float by accident
Also I renamed stacked sorting offset to -> sorting stacked order
to more easily imply what it does

@Faolan-Rad Faolan-Rad force-pushed the master branch 2 times, most recently from 75dc7dc to 267a6e5 Compare May 20, 2025 06:12
@Faolan-Rad Faolan-Rad requested review from a team as code owners May 20, 2025 06:12
@Faolan-Rad Faolan-Rad force-pushed the master branch 2 times, most recently from 96eb98c to d816f3d Compare May 20, 2025 06:55
Copy link
Member

@clayjohn clayjohn 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 provide some details about why you added sorting_stacked_order it seems to solve the same issue that the existing sort_offset does. We discussed doing something similar to sorting_stacked_order but decided to go with the sort_offset instead, although I can't remember the exact reason.

@Faolan-Rad
Copy link
Contributor Author

Could you provide some details about why you added sorting_stacked_order

So in my use case I am implementing a 3D UI for VR Talking about the problem its also disgusting to use a float in order to control z ordering

@Lielay9
Copy link
Contributor

Lielay9 commented May 21, 2025

Could you provide some details about why you added sorting_stacked_order it seems to solve the same issue that the existing sort_offset does. We discussed doing something similar to sorting_stacked_order but decided to go with the sort_offset instead, although I can't remember the exact reason.

Coming from the perspective of 2.5D game (flat entities rendered in transparent pass in 3D environment):
The sort_offset is quite unwieldy in many cases as adjusting it in one place has the chain reaction of effectively requiring changing it on every transparent mesh. There are many cases where'd you want something to be on the same depth plane but have control over the order. I for example layer entities in the following way: player characters on top of enemies, and elite enemies on top of basic enemies etc. To have this kind of layering with sort_offset all transparent meshes need to be manually sorted by depth so that the entities don't overlap the game environment or other entities closer to the camera.

The sorting_stacked_order here is far better suitable for this purpose as no sorting is needed, the entities can simply be organized based on their priority to their own sorting_stacked_order value. This is also makes dynamically sorting/layering the entities far easier, for example layering the entities based on their remaining health.

@Faolan-Rad
Copy link
Contributor Author

I have updated my test
project.zip

@Faolan-Rad
Copy link
Contributor Author

@clayjohn I have implemented a new sort key for transparent explicitly and for stacked order I just flip the sign bit so the order works the only thing I am a bit questionable on is if I should just do bitwise equal for the same depth check it will work for the submeshes and and other stuff because they share an owner but for meshes that are literately on top of each other it could brake given how they got on top of each other to be honest I am sort of 50 50 on it

@Faolan-Rad
Copy link
Contributor Author

Also this system is the precursor to transparency groups all you would need to do for a group is a system to make all objects within the group use the same depth (user set, closest object, farthest object) doesn't really matter just needs the depth for the point of injection and then the user uses stacked order to actually handle the order within the group

@Faolan-Rad Faolan-Rad requested a review from clayjohn May 22, 2025 04:16
@Faolan-Rad
Copy link
Contributor Author

Now uses a single sort key for transparent sorting

@Faolan-Rad
Copy link
Contributor Author

@clayjohn I believe this is done now unless you can think of anything I could be missing

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected in Forward+/Mobile.

I think the new property makes sense, as it's useful for 2.5D usage with composited Sprite3Ds.

There should be a configuration warning when the property is set to a value other than 0 when using the Compatibility rendering method though, as it's not implemented there.

@clayjohn
Copy link
Member

@clayjohn I believe this is done now unless you can think of anything I could be missing

I should have caught this earlier (Calinou's comment made me realize), but this same change should be added for the Compatibility renderer too. The code is very similar to the mobile and forward+ renderers, so the implementation can basically be copied over.

@AThousandShips AThousandShips changed the title Improved render sorting for forward renders Improve render sorting for forward renders May 28, 2025
@clayjohn
Copy link
Member

I brought this up in today's rendering meeting and the team expressed a bit of concern about adding yet another tool to manage rendering order in the transparent pipeline. Right now we have render priority, sort offset and aabb center. We want to avoid a situation where there are multiple competing solutions to the same problem. We ran out of time before we could fully discuss the problem and whether sorting_stacked_order is the proper solution we want to go forward with.

Unfortunately since this PR didn't link any of the related proposals, we didn't have those in front of us to properly judge whether the current tools available are enough for users.

I think we will need to discuss this again at the meeting in two weeks with the proposals in hand to properly decide what to do. Personally I am leaning towards merging the implementation in this form (once Compatibility renderer support is added), but I want to make sure everyone is happy with that approach.

For future reference, here are the proposals that relate to this:
godotengine/godot-proposals#12214
godotengine/godot-proposals#3971 (proposal is for 2D, but discussion includes 3D)
godotengine/godot-proposals#3986

@Faolan-Rad Faolan-Rad requested a review from a team as a code owner May 30, 2025 10:47
@Faolan-Rad
Copy link
Contributor Author

I did add a MSVC warning ignore in cowdata because after rebase I was no longer able to compile locally because of it. I did update MSVC so I don't know if it was a problem was with a recent commit or a new thing with the newer version of MSVC
I added the warning suppression in cowdata stating it is a false positive mod by 0

@Ivorforce
Copy link
Member

I did add a MSVC warning ignore in cowdata because after rebase I was no longer able to compile locally because of it. I did update MSVC so I don't know if it was a problem was with a recent commit or a new thing with the newer version of MSVC I added the warning suppression in cowdata stating it is a false positive mod by 0

That's fine, CowData is known to produce false positive warnings.

@Faolan-Rad Faolan-Rad changed the title Improve render sorting for forward renders Improve render sorting for alpha pass May 30, 2025
@Faolan-Rad
Copy link
Contributor Author

@clayjohn I added compatible render I believe at this point the pr is completely ready

@Lielay9
Copy link
Contributor

Lielay9 commented Jun 23, 2025

FYI there's a merge conflict.

Made another project for specifically testing the order of meshes on the same plane. Once again, coming from the perspective of 2.5D, the predictable and tunable order this pr brings is a great improvement. I have forked and used this pr now for a couple weeks and the implementation seems solid.

alpha-sort-demo.zip:

stable_sort_order_demo

All colored squares are on the same plane. Disregard the left side, the sorting in pr does work correctly; for the left and 2.5D games at large to work you'd also need the option for depth be calculated using custom plane/normal.

@Faolan-Rad
Copy link
Contributor Author

Faolan-Rad commented Jun 23, 2025

All colored squares are on the same plane. Disregard the left side, the sorting in pr does work correctly; for the left and 2.5D games at large to work you'd also need the option for depth be calculated using custom plane/normal.

I think the thing that would make it easier is transparency groups where you put all of the objects in a group which makes them all the same depth and with that you set it to use the closest object depth farthest mid point or average. So in your use case each layer/plane you want you put them all in a group

I do think a definable pipeline for depth to override how it works on a per object basis would be nice which could even be the system used to make the transparency groups

@Faolan-Rad
Copy link
Contributor Author

Should I just already add the system for transparency groups which would make the use of stacked sorting order. I would add the nearest, farthest, average which is the distance chosen for the group of transparent objects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants