-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Improve sort order #5341
Improve sort order #5341
Changes from 15 commits
a779d17
8e1ab6f
0014eef
4d85673
29010da
2b708f6
1098756
6010d1e
5ea0f8d
41f016d
70b98c1
fb4caa4
9f22be5
9462f1c
d640779
2d93a76
589c4ec
1b33604
16f4e26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ module.exports.System = registerSystem('renderer', { | |
toneMapping: {default: 'no', oneOf: ['no', 'ACESFilmic', 'linear', 'reinhard', 'cineon']}, | ||
precision: {default: 'high', oneOf: ['high', 'medium', 'low']}, | ||
anisotropy: {default: 1}, | ||
sortObjects: {default: false}, | ||
sortTransparentObjects: {default: false}, | ||
colorManagement: {default: true}, | ||
alpha: {default: true}, | ||
foveationLevel: {default: 1} | ||
|
@@ -32,7 +32,7 @@ module.exports.System = registerSystem('renderer', { | |
var toneMappingName = this.data.toneMapping.charAt(0).toUpperCase() + this.data.toneMapping.slice(1); | ||
// This is the rendering engine, such as THREE.js so copy over any persistent properties from the rendering system. | ||
var renderer = sceneEl.renderer; | ||
renderer.sortObjects = data.sortObjects; | ||
|
||
renderer.useLegacyLights = !data.physicallyCorrectLights; | ||
renderer.toneMapping = THREE[toneMappingName + 'ToneMapping']; | ||
THREE.Texture.DEFAULT_ANISOTROPY = data.anisotropy; | ||
|
@@ -47,6 +47,10 @@ module.exports.System = registerSystem('renderer', { | |
if (sceneEl.hasAttribute('logarithmicDepthBuffer')) { | ||
warn('Component `logarithmicDepthBuffer` is deprecated. Use `renderer="logarithmicDepthBuffer: true"` instead.'); | ||
} | ||
|
||
// These properties are always the same, regardless of rendered oonfiguration | ||
renderer.sortObjects = true; | ||
renderer.setOpaqueSort(sortOpaqueDefault); | ||
}, | ||
|
||
update: function () { | ||
|
@@ -57,6 +61,15 @@ module.exports.System = registerSystem('renderer', { | |
renderer.toneMapping = THREE[toneMappingName + 'ToneMapping']; | ||
renderer.toneMappingExposure = data.exposure; | ||
renderer.xr.setFoveation(data.foveationLevel); | ||
|
||
if (data.sortObjects) { | ||
warn('`sortObjects` property is deprecated. Use `renderer="sortTransparentObjects: true"` instead.'); | ||
} | ||
if (data.sortTransparentObjects) { | ||
renderer.setTransparentSort(sortTransparentSpatial); | ||
} else { | ||
renderer.setTransparentSort(sortTransparentDefault); | ||
} | ||
}, | ||
|
||
applyColorCorrection: function (texture) { | ||
|
@@ -83,3 +96,59 @@ module.exports.System = registerSystem('renderer', { | |
} | ||
} | ||
}); | ||
|
||
// Custom A-Frame sort functions. | ||
// Variations of Three.js default sort orders here: | ||
// https://github.com/mrdoob/three.js/blob/ebbaecf9acacf259ea9abdcba7b6fb25cfcea2ab/src/renderers/webgl/WebGLRenderLists.js#L1 | ||
// See: https://github.com/aframevr/aframe/issues/5332 | ||
|
||
// Default sort for opaque objects: | ||
// - respect groupOrder & renderOrder settings | ||
// - 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) { | ||
if (a.groupOrder !== b.groupOrder) { | ||
return a.groupOrder - b.groupOrder; | ||
} else if (a.renderOrder !== b.renderOrder) { | ||
return a.renderOrder - b.renderOrder; | ||
} else if (a.z !== b.z) { | ||
return a.z - b.z; | ||
} else { | ||
return 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this statement might be redundant. If one does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similarly:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it make sense to retain the ID tie-breaker ( 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:
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
} | ||
} | ||
|
||
// Default sort for transparent objects: | ||
// - respect groupOrder & renderOrder settings | ||
// - otherwise leave objects in default order (object tree order) | ||
function sortTransparentDefault (a, b) { | ||
if (a.groupOrder !== b.groupOrder) { | ||
return a.groupOrder - b.groupOrder; | ||
} else if (a.renderOrder !== b.renderOrder) { | ||
return a.renderOrder - b.renderOrder; | ||
} else { | ||
return 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you could do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
} | ||
|
||
// Spatial sort for transparent objects: | ||
// - respect groupOrder & renderOrder settings | ||
// - sort back-to-front by z-depth from camera | ||
// - otherwise leave objects in default order (object tree order) | ||
function sortTransparentSpatial (a, b) { | ||
if (a.groupOrder !== b.groupOrder) { | ||
return a.groupOrder - b.groupOrder; | ||
} else if (a.renderOrder !== b.renderOrder) { | ||
return a.renderOrder - b.renderOrder; | ||
} else if (a.z !== b.z) { | ||
return b.z - a.z; | ||
} else { | ||
return 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similarly:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
} | ||
|
||
// exports needed for Unit Tests | ||
module.exports.sortOpaqueDefault = sortOpaqueDefault; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
module.exports.sortTransparentDefault = sortTransparentDefault; | ||
module.exports.sortTransparentSpatial = sortTransparentSpatial; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,10 +46,12 @@ setup(function () { | |
}, | ||
dispose: function () {}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reordered this list to be alphabetic |
||
getContext: function () { return undefined; }, | ||
render: function () {}, | ||
setAnimationLoop: function () {}, | ||
setSize: function () {}, | ||
setOpaqueSort: function () {}, | ||
setPixelRatio: function () {}, | ||
render: function () {}, | ||
setSize: function () {}, | ||
setTransparentSort: function () {}, | ||
shadowMap: {enabled: false} | ||
}; | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,7 @@ | ||
/* global assert, suite, test, setup, teardown, THREE */ | ||
var {sortOpaqueDefault, | ||
sortTransparentDefault, | ||
sortTransparentSpatial} = require('systems/renderer'); | ||
|
||
suite('renderer', function () { | ||
function createScene () { | ||
|
@@ -9,22 +12,26 @@ suite('renderer', function () { | |
|
||
test('default initialization', function (done) { | ||
var sceneEl = createScene(); | ||
var sortFunction; | ||
sceneEl.renderer.setOpaqueSort = function (s) { sortFunction = s; }; | ||
sceneEl.addEventListener('loaded', function () { | ||
// Verify the properties which are part of the renderer system schema. | ||
var rendererSystem = sceneEl.getAttribute('renderer'); | ||
assert.strictEqual(rendererSystem.foveationLevel, 1); | ||
assert.strictEqual(rendererSystem.highRefreshRate, false); | ||
assert.strictEqual(rendererSystem.physicallyCorrectLights, false); | ||
assert.strictEqual(rendererSystem.sortObjects, false); | ||
assert.strictEqual(rendererSystem.sortTransparentObjects, false); | ||
assert.strictEqual(rendererSystem.colorManagement, true); | ||
assert.strictEqual(rendererSystem.anisotropy, 1); | ||
|
||
// Verify properties that are transferred from the renderer system to the rendering engine. | ||
var renderingEngine = sceneEl.renderer; | ||
assert.strictEqual(renderingEngine.outputColorSpace, THREE.SRGBColorSpace); | ||
assert.notOk(renderingEngine.sortObjects); | ||
assert.ok(renderingEngine.sortObjects); | ||
assert.strictEqual(sortFunction, sortOpaqueDefault); | ||
assert.strictEqual(renderingEngine.useLegacyLights, true); | ||
assert.strictEqual(THREE.Texture.DEFAULT_ANISOTROPY, 1); | ||
|
||
done(); | ||
}); | ||
document.body.appendChild(sceneEl); | ||
|
@@ -41,11 +48,26 @@ suite('renderer', function () { | |
document.body.appendChild(sceneEl); | ||
}); | ||
|
||
test('change renderer sortObjects', function (done) { | ||
test('change renderer sortTransparentObjects', function (done) { | ||
var sceneEl = createScene(); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that Unit Tests mock out the Should we be looking for a way to improve UT coverage of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that the configuration of the renderer is a bit split between both
This unit test now also tests that the sorting behaviour is correct, but I would split that off into a new Finding a way to improve test coverage of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pushed some changes to address these comments...
The renderer With the updated code, Sorting functions are set in the renderer
Removed that check from the test.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 That does leave us in the situation where renderer initialization is effectively split in two parts That being said, the actual There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I completely agree, as most of the 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, then everything is good to go IMO :-) |
||
var sortFunction; | ||
sceneEl.renderer.setTransparentSort = function (s) { sortFunction = s; }; | ||
sceneEl.setAttribute('renderer', 'sortTransparentObjects: true;'); | ||
sceneEl.addEventListener('loaded', function () { | ||
assert.strictEqual(sortFunction, sortTransparentSpatial); | ||
done(); | ||
}); | ||
document.body.appendChild(sceneEl); | ||
}); | ||
|
||
test('default renderer sortTransparentObjects', function (done) { | ||
var sceneEl = createScene(); | ||
sceneEl.setAttribute('renderer', 'sortObjects: true;'); | ||
|
||
var sortFunction; | ||
sceneEl.renderer.setTransparentSort = function (s) { sortFunction = s; }; | ||
sceneEl.addEventListener('loaded', function () { | ||
assert.ok(sceneEl.renderer.sortObjects); | ||
assert.strictEqual(sortFunction, sortTransparentDefault); | ||
done(); | ||
}); | ||
document.body.appendChild(sceneEl); | ||
|
@@ -208,4 +230,81 @@ suite('renderer', function () { | |
console.warn = oldConsoleWarn; | ||
}); | ||
}); | ||
|
||
suite('sortFunctions', function () { | ||
var objects; | ||
var objectsRenderOrder; | ||
var objectsGroupOrder; | ||
|
||
setup(function () { | ||
objects = [ | ||
{ name: 'a', renderOrder: 0, z: 1 }, | ||
{ name: 'b', renderOrder: 0, z: 3 }, | ||
{ name: 'c', renderOrder: 0, z: 2 } | ||
]; | ||
|
||
objectsRenderOrder = [ | ||
{ name: 'a', renderOrder: 1, z: 1 }, | ||
{ name: 'b', renderOrder: 0, z: 3 }, | ||
{ name: 'c', renderOrder: -1, z: 2 } | ||
]; | ||
|
||
objectsGroupOrder = [ | ||
{ name: 'a', groupOrder: 0, renderOrder: 1, z: 1 }, | ||
{ name: 'b', groupOrder: 0, renderOrder: 0, z: 3 }, | ||
{ name: 'c', groupOrder: 1, renderOrder: -1, z: 2 } | ||
]; | ||
}); | ||
|
||
function checkOrder (objects, array) { | ||
array.forEach((item, index) => { | ||
assert.equal(objects[index].name, item); | ||
}); | ||
} | ||
|
||
test('Opaque sort sorts front-to-back', function () { | ||
objects.sort(sortOpaqueDefault); | ||
checkOrder(objects, ['a', 'c', 'b']); | ||
}); | ||
|
||
test('Opaque sort respects renderOrder', function () { | ||
objectsRenderOrder.sort(sortOpaqueDefault); | ||
checkOrder(objectsRenderOrder, ['c', 'b', 'a']); | ||
}); | ||
|
||
test('Opaque sort respects groupOrder, then renderOrder', function () { | ||
objectsGroupOrder.sort(sortOpaqueDefault); | ||
checkOrder(objectsGroupOrder, ['b', 'a', 'c']); | ||
}); | ||
|
||
test('Transparent default sort sorts in DOM order', function () { | ||
objects.sort(sortTransparentDefault); | ||
checkOrder(objects, ['a', 'b', 'c']); | ||
}); | ||
|
||
test('Transparent default sort respects renderOrder', function () { | ||
objectsRenderOrder.sort(sortTransparentDefault); | ||
checkOrder(objectsRenderOrder, ['c', 'b', 'a']); | ||
}); | ||
|
||
test('Transparent default sort respects groupOrder, then renderOrder', function () { | ||
objectsGroupOrder.sort(sortTransparentDefault); | ||
checkOrder(objectsGroupOrder, ['b', 'a', 'c']); | ||
}); | ||
|
||
test('Transparent spatial sort sorts back-to-front', function () { | ||
objects.sort(sortTransparentSpatial); | ||
checkOrder(objects, ['b', 'c', 'a']); | ||
}); | ||
|
||
test('Transparent spatial sort respects renderOrder', function () { | ||
objectsRenderOrder.sort(sortTransparentSpatial); | ||
checkOrder(objectsRenderOrder, ['c', 'b', 'a']); | ||
}); | ||
|
||
test('Transparent spatial sort respects groupOrder, then renderOrder', function () { | ||
objectsGroupOrder.sort(sortTransparentSpatial); | ||
checkOrder(objectsGroupOrder, ['b', 'a', 'c']); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can make this function names a bit more descriptive this is almost ready to merge. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).