test(ChunkMeshWorker): initial attempt at using reactor-test #4987

keturn · 2022-01-20T06:00:53Z

Use reactor-test methods to write these tests of ChunkMeshWorker's Flux. Targets #4786.

Key Points to Review:

Are the tests legible? Can you tell what they are supposed to be testing and what they expect?
I tried to centralize the responsibility for calling mesh.dispose by putting that in Chunk#setMesh : 72b0f67 (#4987), does that make sense?
- Is that an appropriate use of AtomicReference? Or is it giving me a false sense of security without preventing any real race condition?

#ci-skip

keturn · 2022-01-21T18:24:57Z

Reactor: ReadWriteStorageManager engine (persistence)

keturn · 2022-01-21T18:26:56Z

Reactor: ChunkMesh engine (rendering)

They are beginning to look readable.

…est/chunkmeshworker

As it is now, being able to pass a different source flux there did not improve testability.

Every usage of setMesh had this, so it seemed safer to make Chunk responsible for this operation.

keturn · 2022-02-06T17:51:35Z

There are still some tests left to complete, but as this is a PR-to-a-PR and I think the tests are more or less legible now, I think this is ready for review.

keturn · 2022-02-06T18:37:03Z

engine/src/main/java/org/terasology/engine/rendering/world/ChunkMeshWorker.java

            ChunkView chunkView = worldProvider.getLocalView(chunk.getPosition());
-            if (chunkView != null && chunkView.isValidView() /* && chunkMeshProcessing.remove(chunk.getPosition()) */) {
+            if (chunkView != null && chunkView.isValidView()) {


FIXME (or test): omitting the chunkMeshProcessing part of this if condition changes something about the reliability of ChunkMeshWorker.remove, I think. Because now there can be things that were removed and are no longer in chunkMeshProcessing list, yet they may still run this code.

Is that harmful? Unsure.

keturn · 2022-02-06T19:57:44Z

Brain dumping on ChunkMeshWorkerTest as it's been a couple weeks:

ChunkMeshWorker has a few collections of state that say something about the view of the world or work-in-progress

Set chunkMeshProcessing
List chunksInProximityOfCamera

in addition to

Chunk.isDirty

Those are the things I'm concerned with having tests for, to make sure we're updating them correctly and they are being used effectively.

One of the things chunkMeshProcessing is being used for is avoiding duplicate work:
if two events throw the same chunk (or location) in the queue twice before the first is processed,
we only need handle it once. Doing it twice would be redundant at best,
as it would be working from the same data at approximately the same time.

It might also be serving as a lock to avoid race conditions?

Things with mutable state that might get racy:

a Chunk has block data
a Chunk has a Mesh, which may be replaced or removed
a ChunkMesh may or may not have some initialization data (vertexElements) and its own buffers and GL buffers which manages.

Some things are not a big deal if they are temporarily inconsistent, like a slight lag in reflecting the current block data.

Other things scare me more, like trying to regenerate ChunkMesh VBOs at the same time another thread is uploading its buffers.

keturn · 2022-02-06T20:07:33Z

ChunkMesh tests

…ration-flowable' into test/chunkmeshworker

keturn · 2022-02-18T20:51:05Z

I got to talk this through with @outofculture. The conclusion we came to (which isn't 100% solid but seems like the intended design) is that ChunkMeshWorker always makes new ChunkMesh objects. And the ChunkMesh that ends up on RenderableChunk.mesh is, in effect, a read-only ChunkMesh.

(In practice, it looks like LodChunkProvider does do chunk.mesh.updateMesh() on a previous mesh, but do LoD chunks get mixed in with other chunks?)

If that's the case, I think I can replace some of the test cases I'm worried about writing with notes to use read-only views.

… optimizations we probably don't need

keturn · 2022-02-18T23:03:49Z

Thoughts on separating out the mutable methods of ChunkMesh are in #4998. It seems like it's probably viable, but some FallingBlocks-related interfaces make it outside the scope of including in this PR.

jdrueckert · 2022-02-19T13:48:46Z

engine-tests/src/test/java/org/terasology/engine/rendering/world/ChunkMeshWorkerTest.java

+                    worker.add(chunk2);
+                    worker.update();
+                })
+                .expectNext(chunk1, chunk2)


does this care about the order? if so, we could include that in the test name, e.g. testMultipleChunksOrdered or something similar

Logically, I don't want to assert order here, but it looks like those arguments to expectNext are ordered. Hmm.

This is the thing I'm most concerned with from this review.

I came up with something that's probably technically correct, but the readability is horrible. Hopefully someone will answer this post with better ideas: How to make order-independent assertions on Flux output?

The author answered my post and I've made some changes accordingly.

I'm torn, because I think using the getChunksThatResultFrom helper I added works well for these cases where you just want the final output and examine all if it together.

And I think StepVerifier is the right sort of tool to use for some of these cases where we're worried about how things change if you add more work while other things are still in progress.

But having some tests written one way and some tests written the other, and having these two different ways of getting a ChunkMeshWorker—I don't feel good about it.

I think, though, that after I add some documentation for the new function, it'll be Close Enough.

The goal here was to get tests that can keep us in line as we rework this code, and I expect the details of how we test will change as we do that.

jdrueckert · 2022-02-19T13:50:18Z

engine-tests/src/test/java/org/terasology/engine/rendering/world/ChunkMeshWorkerTest.java

+                })
+                .expectNextCount(1).as("expect only one result")
+                .then(() -> {
+                    // adding it again and doing another update should still not change


do we need another assertion after this? 🤔

The verify at the end will fail if another thing comes through that's not expected, with a message like

expected: timeout(5s);
actual: onNext(DummyChunk{chunkPos=( 1.240E+2 4.560E+2 7.890E+2), dirty=false, mesh=Mock for ChunkMesh, hashCode: 1832543155, ready=true})

jdrueckert · 2022-02-19T13:51:23Z

engine-tests/src/test/java/org/terasology/engine/rendering/world/ChunkMeshWorkerTest.java

+                    worker.remove(chunk);
+                    worker.update();
+                })
+                // chunk was removed, no events expected


can we do something like expectNextCount(0) here to assert this?

jdrueckert · 2022-02-19T13:52:44Z

engine-tests/src/test/java/org/terasology/engine/rendering/world/ChunkMeshWorkerTest.java

+                    worker.update();
+                })
+                .then(() -> {
+                    worker.remove(chunk);  // second time calling remove on the same chunk


do we consider the following a different test case?

worker.add(chunk); worker.remove(chunk); worker.remove(chunk); worker.update();

I don't see one, no. Are you concerned we need one?

jdrueckert · 2022-02-19T13:53:00Z

engine-tests/src/test/java/org/terasology/engine/rendering/world/ChunkMeshWorkerTest.java

+                    worker.remove(chunk);  // second time calling remove on the same chunk
+                    worker.update();
+                })
+                // chunk was removed, no events expected


can we do something like expectNextCount(0) here to assert this?

jdrueckert · 2022-02-19T13:53:13Z

engine-tests/src/test/java/org/terasology/engine/rendering/world/ChunkMeshWorkerTest.java

+                    worker.remove(position0);
+                    worker.update();
+                })
+                // chunk was removed, no events expected


…est/chunkmeshworker

…r-independent Because they run in parallel, if there are only two of them, either one may return first.

pollend · 2022-05-06T02:04:30Z

the ways lod's work is kind of not great tbh. not sure if it should be generated as part of a chunk and cached together with the chunk. having to regenerate the chunk seems like it adds quite a lot of bandwidth to the gpu.

…est/chunkmeshworker

test(ChunkMeshWorker): initial attempt at using reactor-test

e4c723a

#ci-skip

keturn mentioned this pull request Jan 21, 2022

refactor(Chunk): provide default implementations of methods where possible #4989

Merged

keturn added 6 commits January 21, 2022 18:21

test(ChunkMeshWorker): implement more tests

e9e32b7

They are beginning to look readable.

Merge branch 'refactor/migrate-chunk-mesh-generation-flowable' into t…

3f5a78f

…est/chunkmeshworker

fix(ChunkMeshWorker): fix no-longer-processing test

cf7710a

refactor(ChunkMeshWorker): inline fluxNewMeshes

095e40d

As it is now, being able to pass a different source flux there did not improve testability.

refactor(Chunk): push dispose-old-mesh logic down in to Chunk.setMesh

72b0f67

Every usage of setMesh had this, so it seemed safer to make Chunk responsible for this operation.

refactor(ChunkMeshWorker): make worker function definitions more compact

ca2f3be

keturn marked this pull request as ready for review February 6, 2022 17:50

keturn added Category: Test/QA Requests, Issues and Changes targeting tests and quality assurance Topic: Concurrency Requests, issues, and changes relating to threading, concurrency, parallel execution, etc. labels Feb 6, 2022

keturn commented Feb 6, 2022

View reviewed changes

Merge remote-tracking branch 'origin/refactor/migrate-chunk-mesh-gene…

430bafa

…ration-flowable' into test/chunkmeshworker

test(ChunkMeshWorker): remove some difficult to implement tests about…

972ee97

… optimizations we probably don't need

keturn mentioned this pull request Feb 18, 2022

refactor(rendering): factor out MutableChunkMesh #4998

Draft

3 tasks

jdrueckert reviewed Feb 19, 2022

View reviewed changes

keturn added 3 commits May 3, 2022 14:00

Merge branch 'refactor/migrate-chunk-mesh-generation-flowable' into t…

4cc9ca1

…est/chunkmeshworker

test(ChunkMeshWorker): make DummyChunk strings slightly more concise

b313fb0

test(ChunkMeshWorker): fix testMultipleChunks so the results are orde…

3db427f

…r-independent Because they run in parallel, if there are only two of them, either one may return first.

keturn added 2 commits May 7, 2022 09:42

test(ChunkMeshWorker): add some documentation

d69eef8

Merge branch 'refactor/migrate-chunk-mesh-generation-flowable' into t…

f9c6475

…est/chunkmeshworker

keturn merged commit cfb307c into refactor/migrate-chunk-mesh-generation-flowable May 7, 2022

keturn deleted the test/chunkmeshworker branch May 7, 2022 17:11

keturn mentioned this pull request May 7, 2022

chore(rendering): migrate chunk mesh generation Flux #4786

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(ChunkMeshWorker): initial attempt at using reactor-test #4987

test(ChunkMeshWorker): initial attempt at using reactor-test #4987

keturn commented Jan 20, 2022 •

edited

Loading

keturn commented Jan 21, 2022

keturn commented Jan 21, 2022

keturn commented Feb 6, 2022

keturn Feb 6, 2022

keturn commented Feb 6, 2022

keturn commented Feb 6, 2022

keturn commented Feb 18, 2022

keturn commented Feb 18, 2022

jdrueckert Feb 19, 2022

keturn Feb 20, 2022

keturn May 3, 2022

keturn May 6, 2022

jdrueckert Feb 19, 2022

keturn Feb 20, 2022

jdrueckert Feb 19, 2022

jdrueckert Feb 19, 2022

keturn Feb 20, 2022

jdrueckert Feb 19, 2022

jdrueckert Feb 19, 2022

pollend commented May 6, 2022

test(ChunkMeshWorker): initial attempt at using reactor-test #4987

test(ChunkMeshWorker): initial attempt at using reactor-test #4987

Conversation

keturn commented Jan 20, 2022 • edited Loading

Key Points to Review:

keturn commented Jan 21, 2022

keturn commented Jan 21, 2022

keturn commented Feb 6, 2022

Choose a reason for hiding this comment

keturn commented Feb 6, 2022

keturn commented Feb 6, 2022

keturn commented Feb 18, 2022

keturn commented Feb 18, 2022

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

Choose a reason for hiding this comment

pollend commented May 6, 2022

keturn commented Jan 20, 2022 •

edited

Loading