-
-
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 14 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 |
---|---|---|
@@ -0,0 +1,56 @@ | ||
// 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) { | ||
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. Function names are a bit cryptic. |
||
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; | ||
} | ||
} | ||
|
||
// 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; | ||
} | ||
} | ||
|
||
// 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; | ||
} | ||
} | ||
|
||
module.exports = { | ||
sortOpaqueDefault: sortOpaqueDefault, | ||
sortTransparentDefault: sortTransparentDefault, | ||
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 |
---|---|---|
@@ -0,0 +1,80 @@ | ||
/* global assert, process, suite, test, setup */ | ||
|
||
var {sortOpaqueDefault, sortTransparentDefault, sortTransparentSpatial} = require('core/scene/sortFunctions'); | ||
|
||
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']); | ||
}); | ||
}); |
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('core/scene/sortFunctions'); | ||
|
||
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); | ||
|
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.
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.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.
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 prependaframe
to the function nameThere 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.
We can think of modularizing later if the functions turn to be useful somewhere else (like those in utils)
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.
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.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 missed they're used in a-scene as well. Do we need them in both places? Can we consolidate in just one?
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.
I don't see the functions used in a-scene. I might be missing something?
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, my mistake. They were used in a-scene until I did some refactoring proposed by @mrxz . I'll fix as you suggest now.