Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add tests for WebGLRenderLists, WebGLRenderList #19346

Merged
merged 7 commits into from Jun 30, 2020
Merged

Add tests for WebGLRenderLists, WebGLRenderList #19346

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c2c73d1
Add tests
gkjohnson May 12, 2020
14cce39
add sort test
gkjohnson May 12, 2020
921cfea
Add "finish" test
gkjohnson May 12, 2020
49d611b
Merge remote-tracking branch 'mrdoob/dev' into webglrenderlists-tests
gkjohnson May 13, 2020
c3698cb
Make requested comment and change for `renderItems`
gkjohnson May 15, 2020
add8912
remove renderItems from webglrenderlists typescript definition
gkjohnson Jun 18, 2020
1f3e799
comment out finish test, remove exposed render items field
gkjohnson Jun 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/renderers/webgl/WebGLRenderLists.js
View file
Open in desktop
Expand Up @@ -159,6 +159,10 @@ function WebGLRenderList() {
}

return {

// Note: renderItems temporarily exposed to enable testing of the `finish` function.

renderItems: renderItems,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we had this before (meaning making something public so it is accessible for unit tests).

@mrdoob Do we need to update the TS declaration file in this case?

Copy link
Owner

@mrdoob mrdoob May 13, 2020 •
edited

Choose a reason for hiding this comment

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

I'm not sure we had this before (meaning making something public so it is accessible for unit tests).

Hmm... I feel like it's worth it.

@mrdoob Do we need to update the TS declaration file in this case?

I don't know. @gkjohnson what do you think?

Copy link
Collaborator

@Mugen87 Mugen87 May 13, 2020 •
edited

Choose a reason for hiding this comment

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

I thought about this tonight and have a concern.

Unit tests should focus on testing the public interface of a component. It's not ideal if internals have to be exposed in order to write "better" unit tests. In general, unit tests should make no assumption about the implementation. Otherwise you have to refactor unit tests when internals change, even if the public interface stays the same.

Hence, I think it's better to not expose renderItems and WebGLRenderList.

Copy link
Collaborator Author

@gkjohnson gkjohnson May 13, 2020 •
edited

Choose a reason for hiding this comment

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

Unit tests should focus on testing the public interface of a component. It's not ideal if internals have to be exposed in order to write "better" unit tests.

Generally I agree but in this case finish can't really be tested otherwise because it has important behavior that otherwise isn't testable from the outside the class -- it's intended to remove references to objects so they can be garbage collected and I think it's valuable to make sure that's working right. If you have other suggestions on how to check that otherwise I'm happy to try another approach!

@mrdoob Do we need to update the TS declaration file in this case?

I don't know. @gkjohnson what do you think?

I think the typescript definitions files are only really useful for classes and members that we expect people to use publicly (it's really just useful for autocomplete and typescript error reporting) so I don't think we need to include it here.

Copy link
Owner

Choose a reason for hiding this comment

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

How about we make these methods public temporarily?
And we add comments in the code to communicate that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about we make these methods public temporarily?

Can you explain what you mean by that? "Temporarily" as in "we'll make them public and see how it goes" and maybe remove them later? Either way I'll add WebGLRenderList and renderItems to the typescript definitions and add a comment saying they were made public for the sake of testing.

Copy link
Owner

Choose a reason for hiding this comment

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

We're making them public so you can get Group.renderOrder fixed. Once it's fixed we can make them private again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So you want to remove the tests for the finish function later, then? To me it makes sense to keep those tests to make sure that finish function doesn't silently break in the future but I'm supportive either way. I've gone ahead and added the comment added renderItems to the typescript definitions.

opaque: opaque,
transparent: transparent,

Expand Down Expand Up @@ -228,4 +232,4 @@ function WebGLRenderLists() {
}


export { WebGLRenderLists };
export { WebGLRenderLists, WebGLRenderList };
350 changes: 341 additions & 9 deletions test/unit/src/renderers/webgl/WebGLRenderLists.tests.js
View file
Open in desktop
Expand Up @@ -3,31 +3,363 @@
*/
/* global QUnit */

import { WebGLRenderLists } from '../../../../../src/renderers/webgl/WebGLRenderLists';
import { WebGLRenderLists, WebGLRenderList } from '../../../../../src/renderers/webgl/WebGLRenderLists';
import { Camera } from '../../../../../src/cameras/Camera';
import { Scene } from '../../../../../src/scenes/Scene';

export default QUnit.module( 'Renderers', () => {

QUnit.module( 'WebGL', () => {

QUnit.module( 'WebGLRenderLists', () => {

// INSTANCING
QUnit.todo( "Instancing", ( assert ) => {
// PUBLIC STUFF
QUnit.test( "get", ( assert ) => {

var renderLists = new WebGLRenderLists();
var sceneA = new Scene();
var sceneB = new Scene();
var cameraA = new Camera();
var cameraB = new Camera();

assert.ok( false, "everything's gonna be alright" );
var listAA = renderLists.get( sceneA, cameraA );
var listAB = renderLists.get( sceneA, cameraB );
var listBA = renderLists.get( sceneB, cameraA );

assert.propEqual( listAA, new WebGLRenderList(), "listAA is type of WebGLRenderList." );
assert.propEqual( listAB, new WebGLRenderList(), "listAB is type of WebGLRenderList." );
assert.ok( listAA !== listAB, "Render lists for camera A and B with same scene are different." );
assert.ok( listAA !== listBA, "Render lists for scene A and B with same camera are different." );
assert.ok( listAA === renderLists.get( sceneA, cameraA ), "The same list is returned when called with the same scene, camera." );

} );

// PUBLIC STUFF
QUnit.todo( "get", ( assert ) => {
QUnit.test( "dispose", ( assert ) => {

var renderLists = new WebGLRenderLists();
var scene = new Scene();
var camera = new Camera();

var list1 = renderLists.get( scene, camera );

scene.dispose()

var list2 = renderLists.get( scene, camera );

assert.ok( list1 !== list2, "New list should be different after disposing of the scene." );

} );

} );


QUnit.module( 'WebGLRenderList', () => {

QUnit.test( 'init', ( assert ) => {

var list = new WebGLRenderList();

assert.ok( list.transparent.length === 0, 'Transparent list defaults to length 0.' );
assert.ok( list.opaque.length === 0, 'Opaque list defaults to length 0.' );

list.push( {}, {}, { transparent: true }, 0, 0, {} );
list.push( {}, {}, { transparent: false }, 0, 0, {} );

assert.ok( list.transparent.length === 1, 'Transparent list is length 1 after adding transparent item.' );
assert.ok( list.opaque.length === 1, 'Opaque list list is length 1 after adding opaque item.' );

list.init();

assert.ok( list.transparent.length === 0, 'Transparent list is length 0 after calling init.' );
assert.ok( list.opaque.length === 0, 'Opaque list list is length 0 after calling init.' );

} );

QUnit.test( 'push', ( assert ) => {

var list = new WebGLRenderList();
var objA = { id: 'A', renderOrder: 0 };
var matA = { transparent: true, program: { id: 1 } };
var geoA = {};

var objB = { id: 'B', renderOrder: 0 };
var matB = { transparent: true, program: { id: 2 } };
var geoB = {};

var objC = { id: 'C', renderOrder: 0 };
var matC = { transparent: false, program: { id: 3 } };
var geoC = {};

var objD = { id: 'D', renderOrder: 0 };
var matD = { transparent: false, program: { id: 4 } };
var geoD = {};

list.push( objA, geoA, matA, 0, 0.5, {} );
assert.ok( list.transparent.length === 1, 'Transparent list is length 1 after adding transparent item.' );
assert.ok( list.opaque.length === 0, 'Opaque list list is length 0 after adding transparent item.' );
assert.deepEqual(
list.transparent[ 0 ],
{
id: 'A',
object: objA,
geometry: geoA,
material: matA,
program: matA.program,
groupOrder: 0,
renderOrder: 0,
z: 0.5,
group: {}
},
'The first transparent render list item is structured correctly.'
);

assert.ok( false, "everything's gonna be alright" );
list.push( objB, geoB, matB, 1, 1.5, {} );
assert.ok( list.transparent.length === 2, 'Transparent list is length 2 after adding second transparent item.' );
assert.ok( list.opaque.length === 0, 'Opaque list list is length 0 after adding second transparent item.' );
assert.deepEqual(
list.transparent[ 1 ],
{
id: 'B',
object: objB,
geometry: geoB,
material: matB,
program: matB.program,
groupOrder: 1,
renderOrder: 0,
z: 1.5,
group: {}
},
'The second transparent render list item is structured correctly.'
);

list.push( objC, geoC, matC, 2, 2.5, {} );
assert.ok( list.transparent.length === 2, 'Transparent list is length 2 after adding first opaque item.' );
assert.ok( list.opaque.length === 1, 'Opaque list list is length 1 after adding first opaque item.' );
assert.deepEqual(
list.opaque[ 0 ],
{
id: 'C',
object: objC,
geometry: geoC,
material: matC,
program: matC.program,
groupOrder: 2,
renderOrder: 0,
z: 2.5,
group: {}
},
'The first opaque render list item is structured correctly.'
);

list.push( objD, geoD, matD, 3, 3.5, {} );
assert.ok( list.transparent.length === 2, 'Transparent list is length 2 after adding second opaque item.' );
assert.ok( list.opaque.length === 2, 'Opaque list list is length 2 after adding second opaque item.' );
assert.deepEqual(
list.opaque[ 1 ],
{
id: 'D',
object: objD,
geometry: geoD,
material: matD,
program: matD.program,
groupOrder: 3,
renderOrder: 0,
z: 3.5,
group: {}
},
'The second opaque render list item is structured correctly.'
);

} );

QUnit.test( 'unshift', ( assert ) => {

var list = new WebGLRenderList();
var objA = { id: 'A', renderOrder: 0 };
var matA = { transparent: true, program: { id: 1 } };
var geoA = {};

var objB = { id: 'B', renderOrder: 0 };
var matB = { transparent: true, program: { id: 2 } };
var geoB = {};

var objC = { id: 'C', renderOrder: 0 };
var matC = { transparent: false, program: { id: 3 } };
var geoC = {};

var objD = { id: 'D', renderOrder: 0 };
var matD = { transparent: false, program: { id: 4 } };
var geoD = {};

list.unshift( objA, geoA, matA, 0, 0.5, {} );
assert.ok( list.transparent.length === 1, 'Transparent list is length 1 after adding transparent item.' );
assert.ok( list.opaque.length === 0, 'Opaque list list is length 0 after adding transparent item.' );
assert.deepEqual(
list.transparent[ 0 ],
{
id: 'A',
object: objA,
geometry: geoA,
material: matA,
program: matA.program,
groupOrder: 0,
renderOrder: 0,
z: 0.5,
group: {}
},
'The first transparent render list item is structured correctly.'
);

list.unshift( objB, geoB, matB, 1, 1.5, {} );
assert.ok( list.transparent.length === 2, 'Transparent list is length 2 after adding second transparent item.' );
assert.ok( list.opaque.length === 0, 'Opaque list list is length 0 after adding second transparent item.' );
assert.deepEqual(
list.transparent[ 0 ],
{
id: 'B',
object: objB,
geometry: geoB,
material: matB,
program: matB.program,
groupOrder: 1,
renderOrder: 0,
z: 1.5,
group: {}
},
'The second transparent render list item is structured correctly.'
);

list.unshift( objC, geoC, matC, 2, 2.5, {} );
assert.ok( list.transparent.length === 2, 'Transparent list is length 2 after adding first opaque item.' );
assert.ok( list.opaque.length === 1, 'Opaque list list is length 1 after adding first opaque item.' );
assert.deepEqual(
list.opaque[ 0 ],
{
id: 'C',
object: objC,
geometry: geoC,
material: matC,
program: matC.program,
groupOrder: 2,
renderOrder: 0,
z: 2.5,
group: {}
},
'The first opaque render list item is structured correctly.'
);

list.unshift( objD, geoD, matD, 3, 3.5, {} );
assert.ok( list.transparent.length === 2, 'Transparent list is length 2 after adding second opaque item.' );
assert.ok( list.opaque.length === 2, 'Opaque list list is length 2 after adding second opaque item.' );
assert.deepEqual(
list.opaque[ 0 ],
{
id: 'D',
object: objD,
geometry: geoD,
material: matD,
program: matD.program,
groupOrder: 3,
renderOrder: 0,
z: 3.5,
group: {}
},
'The second opaque render list item is structured correctly.'
);

} );

QUnit.test( 'sort', ( assert ) => {

var list = new WebGLRenderList();
var items = [ { id: 4 }, { id: 5 }, { id: 2 }, { id: 3 } ];

items.forEach( item => {

list.push( item, {}, { transparent: true }, 0, 0, {} );
list.push( item, {}, { transparent: false }, 0, 0, {} );

} );

list.sort( ( a, b ) => a.id - b.id, ( a, b ) => b.id - a.id );

assert.deepEqual(
list.opaque.map( item => item.id ),
[ 2, 3, 4, 5 ],
'The opaque sort is applied to the opaque items list.'
);

assert.deepEqual(
list.transparent.map( item => item.id ),
[ 5, 4, 3, 2 ],
'The transparent sort is applied to the transparent items list.'
);

} );

QUnit.todo( "dispose", ( assert ) => {
QUnit.test( 'finish', ( assert ) => {

var list = new WebGLRenderList();
var obj = { id: 'A', renderOrder: 0 };
var mat = { transparent: false, program: { id: 0 } };
var geom = {};

assert.ok( list.renderItems.length === 0, 'Render items length defaults to 0.' );

list.push( obj, geom, mat, 0, 0, {} );
list.push( obj, geom, mat, 0, 0, {} );
list.push( obj, geom, mat, 0, 0, {} );
assert.ok( list.renderItems.length === 3, 'Render items length expands as items are added.' );

list.finish();
assert.deepEqual(
list.renderItems.map( item => item.object ),
[ obj, obj, obj ],
'Render items are not cleaned if they are being used.'
);
assert.deepEqual(
list.renderItems[ 1 ],
{
id: 'A',
object: obj,
geometry: geom,
material: mat,
program: mat.program,
groupOrder: 0,
renderOrder: 0,
z: 0,
group: {}
},
'Unused render item is structured correctly before clearing.'
);

list.init();
list.push( obj, geom, mat, 0, 0, {} );
assert.ok( list.renderItems.length === 3, 'Render items length does not shrink.' );

list.finish();
assert.deepEqual(
list.renderItems.map( item => item.object ),
[ obj, null, null ],
'Render items are cleaned if they are not being used.'
);

assert.deepEqual(
list.renderItems[ 1 ],
{
id: null,
object: null,
geometry: null,
material: null,
program: null,
groupOrder: 0,
renderOrder: 0,
z: 0,
group: null
},
'Unused render item is structured correctly before clearing.'
);

assert.ok( false, "everything's gonna be alright" );

} );

Expand Down