VisualServer now sorts based on AABB position

[QbieShay]

this was causing issues with scenes where the origin of the objects
was set for all objects to the center of the scene, making transparent
objects sort improperly

This work was kindly sponsored by IMVU

Co-authored-by: RevoluPowered gordon@gordonite.tech

[lawnjelly]

Is this not sorting by the minimum position of the AABB, rather then the centre (of the AABB)? This is slightly confusing terminology used in the AABB - position is the minimums. You would have to calculate the centre (or you could store it separately if used multiple times, however the calculation isn't that involved).

That said I can see the reasoning for this with off axis models and it does make sense. This also could potentially be beneficial for models that are animated moving from their origin.

Unless this is violating any assumptions elsewhere (I'm not that familiar with the 3d renderer), clayjohn / reduz would have to confirm this.

[QbieShay]

I had considered calculating the center but I'm not familiar enough with the renderer. On one side, I didn't want to add another variable to the struct for the transformed aabb center, on the other hand i wasn't sure if transforming the aabb position would break something else.

[lawnjelly]

Well, simple version, your line would just become something like:

Vector3 aaab_center = ins->transformed_aabb.position + (ins->transformed_aabb.size * 0.5);
ins->depth = near_plane.distance_to(aabb_center);

Using the minimums (position) as you are will result in depth sorting errors skewed to one side in world space. Not that you won't get some visual errors using the centre, but that is unavoidable with such coarse sorting.

[QbieShay]

Ah thank you @lawnjelly . I hadn't seen the transformed aabb size, I thought I'd have to add it. I will update the PR this evening

[QbieShay]

Well, a very very long evening

[akien-mga]

The Git history seems a bit messed up as you reverted a commit by reduz, and amended your work into another one.

[QbieShay]

Oh no 🤦 . Let me fix it

[lawnjelly]

Looking a bit closer I can see a similar calculation is done for shadows and for GI probes. Presumably they would also benefit, and there is a greater case for calculating the centre as a one off.

Maybe we can ask reduz if he agrees with the general approach before going further, just in case he prefers using the origin, and any advice about calculating centre as one off?

EDIT : Actually I have no idea why it is calculating depth for lights, rough depth sorting maybe, or for some cutoff with distance? It shouldn't need to render in depth order for transparency for shadow maps.

[QbieShay]

@lawnjelly sounds good. I have tested this in the bistro scene and it makes the bushes render correctly,whereas before they would oddly overlap and flicker with one another in the area with the big tree (so it does work :) ).

[akien-mga]

Will need a rebase.

Looking a bit closer I can see a similar calculation is done for shadows and for GI probes. Presumably they would also benefit, and there is a greater case for calculating the centre as a one off.

Should this be changed in this PR too? (Once approved by @reduz as discussed)

[lawnjelly]

It's ok to merge as is imo (for 3.2 if reduz wanted to do differently in master). If we decided to add to another part too it could be a separate PR.

[akien-mga]

@QbieShay @godotengine/rendering I can't remember, what was the consensus regarding what to do with the master version of this PR? Is it planned to merge for 4.0 too or should it be closed?

[clayjohn]

@akien-mga I don't remember any issues with this PR. As far as I recall, it was ready to merge.

[lawnjelly]

Was fine as far as I remember.

[akien-mga]

Alright, I thought @reduz wanted to do something different for master but there was some back and forth. Just needs a rebase then :)

[QbieShay]

This is implemented already:https://github.com/godotengine/godot/blob/master/servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp#L923