Improve sort order #5341

diarmidmackenzie · 2023-07-20T09:51:20Z

Description:

A PR to implement the changes proposed in #5332

Changes proposed:

Set sortObjects to true in all cases
Override THREE.js default sort functions with A-Frame sort functions
replace renderer sortObjects property with sortTransparentObjects property that sets a different A-Frame sort function for transparent objects
documentation and Unit Tests.

diarmidmackenzie · 2023-07-20T09:52:54Z

docs/components/renderer.md

-By default, objects are not sorted, and the order of elements in the DOM determines order of
-rendering. Re-ordering DOM elements provides one way of forcing a consistent behavior, whereas
-use of `renderer="sortObjects: true"` may cause unwanted changes as the camera moves.
+In may cases, the order in which objects is rendered doesn't matter much, but there are a few cases where sorting is important:


This is documentation of general A-Frame object sorting behaviour, rather than this particular property.

Not sure if there's a better place for this in the docs, but I couldn't see an obvious one, and didn't want to restructure docs too much.

Small typo: may -> many

The text seems a bit out of place compared to the rest as it's considerably more in-depth and technical. While I personally don't mind it too much, it's usefulness to new(er) users is probably limited as a result. Surprisingly the original text still mostly holds (as it already pertained to transparent objects). Touching that up and linking to a different page or resource describing it more in-depth (preferably with concrete pros and cons) would IMHO be ideal.

What you suggest is the approach I wanted to take, but I couldn't find a good place in the docs to slip in a section on render order. It's too detailed for me to want to put it into the core docs sections that work as a guide for newcomers.

Have looked again and I think the FAQ https://aframe.io/docs/1.4.0/introduction/faq.html is somewhere that could probably accommodate something.

Probably a question "What order does A-Frame render objects in?" though it could also be "Why are my transparent objects blocking other objects?".

Sound OK?

Sounds okay to me, I would go for the "Why are my transparent objects blocking other objects?" or something along those lines. It's closer to the question people likely have when facing the issue, especially if they don't know it has to do with sorting. (And those that do know it has to do with sorting will likely find the sortTransparentObjects property first which links to the relevant FAQ question).

I've left it as "what order..." - there are other ordering issues that are nothing to do with transparency (e.g. the hider material issues that got me into looking at this in the 1st place, and a bunch of the content has nothing to do with transparency).

diarmidmackenzie · 2023-07-20T09:54:13Z

src/core/scene/sortFunctions.js

@@ -0,0 +1,56 @@
+// Custom A-Frame sort functions.


I think putting these into a new module is right. Not sure whether this is the right place in the directory structure, or if there's a more suitable place.

I didn't think these were utils as they aren't part of the exposed A-Frame API. Couldn't see another obvious directory to put them in, so put them here.

I would put them in the renderer file (define at the bottom) where they are used. see setupCanvas function in a-scene https://github.com/aframevr/aframe/blob/master/src/core/scene/a-scene.js#L863 No need to prepend aframe to the function name

We can think of modularizing later if the functions turn to be useful somewhere else (like those in utils)

Are you sure about this? They are used in both a-scene, and renderer. Defining them in renderer means that a core module (a-scene) will have a dependency on a systems module (renderer).

That seems like the wrong direction for a dependency to me.

It might be preferable to define them in a-scene?

Agree with removing aframe from the function names, and have made that change.

Sorry missed they're used in a-scene as well. Do we need them in both places? Can we consolidate in just one?

I don't see the functions used in a-scene. I might be missing something?

Sorry, my mistake. They were used in a-scene until I did some refactoring proposed by @mrxz . I'll fix as you suggest now.

diarmidmackenzie · 2023-07-20T09:57:29Z

tests/systems/renderer.test.js

    var sceneEl = createScene();
-    sceneEl.setAttribute('renderer', 'sortObjects: true;');
+


The fact that Unit Tests mock out the setupRenderer function means that I was limited in how much of the code changes I could Unit Test in a straightforward way.

Should we be looking for a way to improve UT coverage of the setupRenderer function?

It seems that the configuration of the renderer is a bit split between both setupRenderer and the init function of the renderer system. It might be easier to move the setting of sortObjects and sorting functions into the init function as well. That way the tests here can:

Verify that the default values are set properly (currently the test 'default initialization' asserts that sortObjects defaults to false and doesn't fail)

Verify that changing the sortTransparentObjects result in the correct sorting functions being set

This unit test now also tests that the sorting behaviour is correct, but I would split that off into a new sortFunctions.test.js that specifically tests them and let this unit test here cover only the responsibility of the renderer system of setting the right sort functions.

Finding a way to improve test coverage of setupRenderer would still be nice, but I think with the above the need for it in the scope of this change is mostly alleviated

Pushed some changes to address these comments...

It might be easier to move the setting of sortObjects and sorting functions into the init function as well.

The renderer init function will only be called if renderer system is initialized. It does appear that's always the case today (even if the renderer property is not explicitly specified on the scene), but I'm not sure it's good to bake that assumption into core code. So I'd argue these properties should be set by the AScene class, and setupRenderer() seems like the right place to do it.

With the updated code, renderer.sortObjects is always true, and the renderer component has no bearing on it at all, so I'd argue it is cleaner to not have the renderer code touch renderer.sortObjects.

Sorting functions are set in the renderer update function, as well as in setupRenderer(). No reason to move them to init, which just restricts them from being changed later.

currently the test 'default initialization' asserts that sortObjects defaults to false and doesn't fail

Removed that check from the test. renderer.sortObjects no longer has anything to do with the renderer component, so that check doesn't belong here any more.

This unit test now also tests that the sorting behaviour is correct, but I would split that off into a new sortFunctions.test.js that specifically tests them and let this unit test here cover only the responsibility of the renderer system of setting the right sort functions.

That's fair. Since the since the sort functions are internal to A-frame, I hadn't twigged that I can just import them, and then compare the functions directly. Have now done that and it works fine.

A full UTs of the sort functions is also a sensible idea - I have added.

The renderer init function will only be called if renderer system is initialized. It does appear that's the case today, but I'm not sure it's good to bake that assumption into core code. So I'd argue these properties should be set by the AScene class, and setupRenderer() seems like the right place to do it.

I kind of see it the other way around. It's the AScene class that is effectively overstepping its boundaries by depending on both the camera and renderer systems. Conceptually in an ECS if the renderer system would be left out, you'd be left with a headless instance. From a pragmatic and legacy perspective I can see how this situation came to be and it's now unfeasible to every change (lots of code depends on the scene holding the renderer and camera).

That does leave us in the situation where renderer initialization is effectively split in two parts AScene.setupRenderer and renderer.init. The logical split I see at the moment is that setupRenderer only sets the properties needed at WebGLRenderer construction time and renderer.init sets anything that can change 'on the fly'. The object sorting is the only odd one out (even before this PR).

That being said, the actual sortObjects property can be left default (= true) so neither needs to assign anything to this. Unless we explicitly want to assert this default (and potentially shield us against three.js ever changing that default). That only leaves the sorting functions which, given the current state of the code, feel more at home in init to me.

Not sure I completely agree, as most of the renderer settings can't be changed on the fly (according to the docs, at least).

https://aframe.io/docs/1.4.0/components/renderer.html

But happy this is the right direction of travel, so I made the changes you suggested.

Ah, you're right, seems I overlooked the few that are set by renderer but not changeable on the fly. The docs and the schema do paint a clear picture that all these settings do belong to the renderer system, yet a good chunk of them are handled (parsed even) by the AScene, which in an ideal setup wouldn't have been the case, but alas.

Anyway, the PR looks good. Just two more ES6 destructuring assignments in renderer.test.js and sortFunctions.test.js and then it's all good to go IMO.

My understanding was that the ES5 restriction only applies to /src.

Plenty of existing code under /test uses let, const, arrow functions etc.

See e.g. animation.test.js

Well, then everything is good to go IMO :-)

diarmidmackenzie · 2023-07-20T09:57:54Z

tests/__init.test.js

@@ -46,10 +46,12 @@ setup(function () {
    },
    dispose: function () {},


reordered this list to be alphabetic

dmarcos · 2023-07-20T20:30:48Z

@mrxz What are your thoughts? Thanks!

mrxz · 2023-07-21T08:51:35Z

src/systems/renderer.js

@@ -1,6 +1,7 @@
 var registerSystem = require('../core/system').registerSystem;
 var utils = require('../utils/');
 var THREE = require('../lib/three');
+var {aframeSortTransparentDefault, aframeSortTransparentSpatial} = require('../core/scene/sortFunctions');


I believe destructuring is an ES6 feature.

Pushed a fix for this (is it ok to require the same module twice?)

Calling require more than once on the same module gives the same result, but I think it's a tad cleaner to assign it to an intermediate, i.e. var sortFunctions = require('../core/scene/sortFunctions')

mrxz · 2023-07-21T08:59:47Z

src/core/scene/sortFunctions.js

+module.exports = {
+  aframeSortOpaqueDefault,
+  aframeSortTransparentDefault,
+  aframeSortTransparentSpatial


These shorthands are also an ES6 features, I'm afraid...
(Why are we torturing ourselves with this ES5-only policy, again? 😓 )

I'll fix.

FYI, I have been working on a proposal to add linting rules to enforce ES5:
#5218

Blocked on the fact that A-Frame now uses class in several places, which is an ES6 feature.

mrxz · 2023-07-21T09:03:43Z

Looking good, tested it on a couple of scenes and works as expected. Left a couple of comments.

…property description into FAQ

…system

dmarcos · 2023-07-28T18:43:48Z

src/core/scene/sortFunctions.js

+// - sort front-to-back by z-depth from camera (this should minimize overdraw)
+// - otherwise leave objects in default order (object tree order)
+
+function sortOpaqueDefault (a, b) {


Function names are a bit cryptic. sortOpaqueObjectsFirst maybe? Default has not meaning without more context

dmarcos · 2023-07-30T19:14:54Z

src/systems/renderer.js

+}
+
+// exports needed for Unit Tests
+module.exports.sortOpaqueDefault = sortOpaqueDefault;


I appreciate the tests. I'll merge but in general I lean towards just unit testing public APIs. Internal functions are implementation details and tests add friction to modify, refactor, improve the code.

dmarcos · 2023-07-30T19:21:49Z

src/systems/renderer.js

+// - sort front-to-back by z-depth from camera (this should minimize overdraw)
+// - otherwise leave objects in default order (object tree order)
+
+function sortOpaqueDefault (a, b) {


Can we find a bit more descriptive names for these functions? I had suggested sortOpaqueObjectsFirst but not sure if good enough. can suggest other names.

If we can make this function names a bit more descriptive this is almost ready to merge. Thanks!

Sorry for slow response on this.

I just pushed a change that updated:
sortOpaqueDefault -> sortSpatialFrontToBack
sortTransparentDefault -> sortRenderOrderOnly
sortTransparentSpatial -> sortSpatialBackToFront

This describes what the functions implement rather than what they are used for (glossing over the detail that the spatial sorts also sort by renderOrder first, in the interest of keeping the function names at a sensible length).

dmarcos · 2023-08-08T06:26:54Z

src/systems/renderer.js

+// - sort front-to-back by z-depth from camera (this should minimize overdraw)
+// - otherwise leave objects in default order (object tree order)
+
+function sortSpatialFrontToBack (a, b) {


What does spatial means in this context? Maybe redundant? sortFrontToBack maybe simpler?

agree FrontToBack implies Spatial, so it's redundant. I'll fix.

dmarcos · 2023-08-08T06:28:16Z

src/systems/renderer.js

+  } else if (a.z !== b.z) {
+    return a.z - b.z;
+  } else {
+    return 0;


I think this statement might be redundant. If one does a.z - b.z if they are equal they will return 0 as well.

similarly:

return (a.groupOrder - b.groupOrder) || (a.renderOrder - b.renderOrder) || (a.z - b.z);

Wouldn't it make sense to retain the ID tie-breaker (return a.id - b.id) used by Three.js as well? That should ensure that, everything else being equal, the order is consistent.

As for compacting it to a single expression, I personally prefer the current more verbose version. It's easier to see what's going on and it resembles the referenced original Three.js code, making it easy to compare. I did experiment with some very crude benchmarking and while both perform about the same, a significant difference forms once some of the properties are (mostly) consistent. It seems the if-else chain allows for better branch-prediction:

0 groupOrder, 1% renderOrder, random Z: If-else chain faster on Chrome/Quest 2, slower on Firefox

random groupOrder, random renderOrder, random z: If-else chain slightly slower on Chrome and Firefox

1 groupOrder, 0 renderOrder, random z If-else completely dominates the compacted expression, both on Chrome and Firefox

0 groupOrder, 0 renderOrder, random z: About equal, if-else slightly faster on Chrome, but slightly slower on Firefox

Result obviously may vary and I don't think the performance of this sorting function would really matter at the end of the day as most VR experiences tend to have relatively low object counts anyway.

Wouldn't it make sense to retain the ID tie-breaker (return a.id - b.id) used by Three.js as well? That should ensure that, everything else being equal, the order is consistent.

That would sort the objects in creation order, rather than DOM order. My understanding is that returning 0 will leave order as-is, which implies DOM order, which matches current behaviour, is explicable in A-Frame terms, and gives the user a level of control by inserting items earlier in the DOM.

IMO that's superior to object id / creation order.

As for compacting it to a single expression, I personally prefer the current more verbose version.

Thanks for doing the perf testing, looks to me like the more verbose version is both easier to read, and probably also more performant (I don't think random data is particularly realistic, and even in that case, if-else seems to be only marginally slower).

So I think that weighs in favour of the code as-is. Which also matches the equivalent THREE.js code.

There may be a case for eliminating the final if test and simply returning a.z - b.z though, so I'll make that change.

That would sort the objects in creation order, rather than DOM order. My understanding is that returning 0 will leave order as-is, which implies DOM order, which matches current behaviour, is explicable in A-Frame terms, and gives the user a level of control by inserting items earlier in the DOM.

I meant only for the cases where we do sort on Z first, where it would act as a tie-breaker. Returning 0 will indeed leave the order in tact (stable sort), though strictly speaking that is the insertion order in the render list, which, arguable, only happens to match the tree-order in Three.js' current implementation. But I'm fine with leaving it out, it's unlikely to cause any issues and if the behaviour in three ever changes, we can simply follow suit.

dmarcos · 2023-08-08T06:30:54Z

src/systems/renderer.js

+  } else if (a.renderOrder !== b.renderOrder) {
+    return a.renderOrder - b.renderOrder;
+  } else {
+    return 0;


I think you could do return (a.groupOrder - b.groupOrder) || (a.renderOrder !== b.renderOrder)

Per above, I believe this change reduces readability, diverges unnecessarily from equivalent THREE.js code, and doesn't improve (and may even harm) performance. So I'd prefer to leave as-is.

dmarcos · 2023-08-08T06:32:55Z

src/systems/renderer.js

+  } else if (a.z !== b.z) {
+    return b.z - a.z;
+  } else {
+    return 0;


similarly:

return (a.groupOrder - b.groupOrder) || (a.renderOrder - b.renderOrder) || (b.z - a.z);

Per above, I believe this change reduces readability, diverges unnecessarily from equivalent THREE.js code, and doesn't improve (and may even harm) performance. So I'd prefer to leave as-is.

…ation

dmarcos · 2023-08-11T23:06:58Z

Thanks so much for the patience! This is awesome

diarmidmackenzie added 3 commits July 19, 2023 15:40

Updated sort functions for A-Frame

a779d17

Test cases WIP

8e1ab6f

Documentation updates

0014eef

diarmidmackenzie commented Jul 20, 2023

View reviewed changes

diarmidmackenzie marked this pull request as ready for review July 20, 2023 09:58

mrxz reviewed Jul 21, 2023

View reviewed changes

diarmidmackenzie added 11 commits July 21, 2023 16:18

Improve renderer unit tests

4d85673

simplify path

29010da

UTs for new sort functions

2b708f6

Destructuring is not allowed in ES5

1098756

Use longhand initialization of object (shorthand unsupported in ES5)

6010d1e

Move object sorting detail away from renderer sortTransparentObjects …

5ea0f8d

…property description into FAQ

More ES5 compliance clean-up

41f016d

Fix typo may -> many

70b98c1

Move initialization of object sorting config from AScene to renderer …

fb4caa4

…system

sortTransparentObjects can be set dynamically.

9f22be5

rename aframeSortX functions to sortX

9462f1c

dmarcos reviewed Jul 28, 2023

View reviewed changes

Move sortFunctions.js module into systems/renderer.js

d640779

dmarcos reviewed Jul 30, 2023

View reviewed changes

Give sort functions more meaningful names

2d93a76

dmarcos reviewed Aug 8, 2023

View reviewed changes

diarmidmackenzie and others added 3 commits August 8, 2023 15:06

Remove redundant 'Spatial' from function names

589c4ec

Remove unnecessary final else-if branches.

1b33604

Early returns to remove "else if" statements and corresponding indent…

16f4e26

…ation

dmarcos merged commit 3c6cc78 into aframevr:master Aug 11, 2023
1 check failed

mrxz mentioned this pull request Oct 7, 2023

ar-hit-test enabled off makes the entity "lose track" #5324

Closed

arpu mentioned this pull request Jun 1, 2024

WebXR mode broken on Vision Pro with A-Frame 1.6.0 #5530

Closed

mrxz mentioned this pull request Nov 14, 2024

Fix render order of background fade in User Interface example #5608

Merged

		var sceneEl = createScene();
		sceneEl.setAttribute('renderer', 'sortObjects: true;');

		@@ -46,10 +46,12 @@ setup(function () {
		},
		dispose: function () {},

Improve sort order #5341

Improve sort order #5341

Conversation

diarmidmackenzie commented Jul 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diarmidmackenzie Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

dmarcos Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diarmidmackenzie Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

mrxz Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarcos commented Jul 20, 2023

Choose a reason for hiding this comment

diarmidmackenzie Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrxz commented Jul 21, 2023

dmarcos Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diarmidmackenzie Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarcos commented Aug 11, 2023

diarmidmackenzie Jul 20, 2023 •

edited

Loading

dmarcos Jul 27, 2023 •

edited

Loading

diarmidmackenzie Jul 21, 2023 •

edited

Loading

mrxz Jul 21, 2023 •

edited

Loading

diarmidmackenzie Jul 21, 2023 •

edited

Loading

dmarcos Jul 28, 2023 •

edited

Loading

diarmidmackenzie Aug 8, 2023 •

edited

Loading