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.

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

Feature: Extraction LightMerger usage in post-ready step. #4502

Closed
wants to merge 22 commits into from

Conversation

DarkWeird
Copy link
Contributor

@DarkWeird DarkWeird commented Feb 9, 2021

Contains

  • LightMerger runs after chunk is ready now.
  • Simplified ChunkProcessingPipeline.
  • Fixed tests
  • Faster chunk delivery to ready.. especially for tests! (should not stuck at chunk generation)

How to test

  1. Run tests
  2. Run Singleplayer game
    2.1. fly on hspeed, check how fast chunk loadings.
  3. Run Multiplayer
    3.1. fly on hspeed, check how fast chunk loadings.

@DarkWeird DarkWeird added Category: Performance Requests, Issues and Changes targeting performance Type: Improvement Request for or addition/enhancement of a feature Topic: Stabilization Requests, Issues and Changes related to improving stablity and reducing flakyness labels Feb 9, 2021
@DarkWeird DarkWeird requested a review from skaldarnar February 9, 2021 15:10
@skaldarnar
Copy link
Member

When I try to start a new game with just Core gameplay it crashes with:

20:22:09.333 [main] ERROR o.t.e.event.internal.EventSystemImpl - Failed to invoke event
java.lang.IllegalArgumentException: Cannot find field java.util.concurrent.CompletableFuture$AsyncSupply.task
	at org.terasology.utilities.ReflectionUtil.readField(ReflectionUtil.java:474)
	at org.terasology.world.chunks.pipeline.ChunkProcessingPipeline.lambda$unwrappingComporator$0(ChunkProcessingPipeline.java:70)
	at java.base/java.util.concurrent.PriorityBlockingQueue.siftUpUsingComparator(PriorityBlockingQueue.java:377)
	at java.base/java.util.concurrent.PriorityBlockingQueue.offer(PriorityBlockingQueue.java:488)
	at java.base/java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1347)
	at java.base/java.util.concurrent.CompletableFuture.asyncSupplyStage(CompletableFuture.java:1714)
	at java.base/java.util.concurrent.CompletableFuture.supplyAsync(CompletableFuture.java:1931)
	at org.terasology.world.chunks.pipeline.ChunkProcessingPipeline.invokeGeneratorTask(ChunkProcessingPipeline.java:107)
	at org.terasology.world.chunks.localChunkProvider.LocalChunkProvider.createOrLoadChunk(LocalChunkProvider.java:122)
	at org.terasology.world.chunks.localChunkProvider.RelevanceSystem.lambda$addRelevanceEntity$0(RelevanceSystem.java:184)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at java.base/java.util.stream.SortedOps$RefSortingSink.end(SortedOps.java:395)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:485)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497)
	at org.terasology.world.chunks.localChunkProvider.RelevanceSystem.addRelevanceEntity(RelevanceSystem.java:178)
	at org.terasology.logic.players.PlayerSystem.addRelevanceEntity(PlayerSystem.java:197)
	at org.terasology.logic.players.PlayerSystem.onConnect(PlayerSystem.java:146)
	at org.terasology.logic.players.PlayerSystemMethodAccess.invoke(Unknown Source)
	at org.terasology.entitySystem.event.internal.EventSystemImpl$ByteCodeEventHandlerInfo.invoke(EventSystemImpl.java:536)
	at org.terasology.entitySystem.event.internal.EventSystemImpl.sendStandardEvent(EventSystemImpl.java:292)
	at org.terasology.entitySystem.event.internal.EventSystemImpl.send(EventSystemImpl.java:283)
	at org.terasology.entitySystem.entity.internal.BaseEntityRef.send(BaseEntityRef.java:205)
	at org.terasology.network.internal.NetworkSystemImpl.connectClient(NetworkSystemImpl.java:882)
	at org.terasology.network.internal.NetworkSystemImpl.joinLocal(NetworkSystemImpl.java:326)
	at org.terasology.engine.modes.loadProcesses.SetupLocalPlayer.step(SetupLocalPlayer.java:45)
	at org.terasology.engine.modes.StateLoading.update(StateLoading.java:237)
	at org.terasology.engine.TerasologyEngine.tick(TerasologyEngine.java:496)
	at org.terasology.engine.TerasologyEngine.mainLoop(TerasologyEngine.java:459)
	at org.terasology.engine.TerasologyEngine.runMain(TerasologyEngine.java:435)
	at org.terasology.engine.TerasologyEngine.run(TerasologyEngine.java:401)
	at org.terasology.engine.Terasology.main(Terasology.java:174)
20:22:09.353 [main] ERROR o.t.engine.modes.StateLoading - Error while loading org.terasology.engine.modes.loadProcesses.AwaitCharacterSpawn@11c40a65
java.lang.IllegalArgumentException: Cannot find field java.util.concurrent.CompletableFuture$AsyncSupply.task
	at org.terasology.utilities.ReflectionUtil.readField(ReflectionUtil.java:474)
	at org.terasology.world.chunks.pipeline.ChunkProcessingPipeline.lambda$unwrappingComporator$0(ChunkProcessingPipeline.java:70)
	at java.base/java.util.concurrent.PriorityBlockingQueue.siftUpUsingComparator(PriorityBlockingQueue.java:377)
	at java.base/java.util.concurrent.PriorityBlockingQueue.offer(PriorityBlockingQueue.java:488)
	at java.base/java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1347)
	at java.base/java.util.concurrent.CompletableFuture.asyncSupplyStage(CompletableFuture.java:1714)
	at java.base/java.util.concurrent.CompletableFuture.supplyAsync(CompletableFuture.java:1931)
	at org.terasology.world.chunks.pipeline.ChunkProcessingPipeline.invokeGeneratorTask(ChunkProcessingPipeline.java:107)
	at org.terasology.world.chunks.localChunkProvider.LocalChunkProvider.createOrLoadChunk(LocalChunkProvider.java:122)
	at org.terasology.world.chunks.localChunkProvider.RelevanceSystem.updateRelevance(RelevanceSystem.java:134)
	at org.terasology.world.chunks.localChunkProvider.RelevanceSystem.update(RelevanceSystem.java:219)
	at org.terasology.engine.modes.loadProcesses.AwaitCharacterSpawn.step(AwaitCharacterSpawn.java:50)
	at org.terasology.engine.modes.StateLoading.update(StateLoading.java:237)
	at org.terasology.engine.TerasologyEngine.tick(TerasologyEngine.java:496)
	at org.terasology.engine.TerasologyEngine.mainLoop(TerasologyEngine.java:459)
	at org.terasology.engine.TerasologyEngine.runMain(TerasologyEngine.java:435)
	at org.terasology.engine.TerasologyEngine.run(TerasologyEngine.java:401)
	at org.terasology.engine.Terasology.main(Terasology.java:174)

@skaldarnar
Copy link
Member

🚀 The chunks indeed load faster, but ...

  • ... it feels like too much is loaded at once, causing a short freeze/frame drop 🙈
  • ... there is a "shadow" border at the edge of the region of loaded chunks that does not go away. It retracts farther away if I move in that direction, but then just the next line of chunks is just shadowy...
    Terasology-210210232308-1973x1031

@DarkWeird
Copy link
Contributor Author

Freeze happened,
because final part processing at main thread with event sending takes many time,
because processed in place, because it is main thread.

Shadowy chunks.
Seems this is border at non-lod chunks.
Yeah. Light merging cannot process this chunks, because not enough data for it.
But strange that this chunks not received light data from internal light grnerator...

@DarkWeird
Copy link
Contributor Author

DarkWeird commented Feb 11, 2021

Oh.
Voxel*WorldSystem takes many time for almost useless work at ChunkLoading...

when chunk loads - VoxelSystem getAll blocks from chunk..
but used only Id all time.. and tryToRegister block for voxelinfo(liquid/solid/penetrable/etc) this part should use only 1 time on every ID.

But VoxelSystems requests block every block position...
Both VoxelSystems!

Suggestion:

  1. Provide method Chunk#getBlockId(x,y,z)
  2. variants:
    2.1. use BlockRegistrationListener
    2.2 wait for Disable dynamic block allocation. #4036 and get block data in game start, where this info is available.

especially now, when chunks delivering faster.

Copy link
Member

@skaldarnar skaldarnar left a comment

Choose a reason for hiding this comment

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

I'm not really the best reviewer for this, I guess (peeking at @4Denthusiast 🙄 ), but I found at least one little bug with iterating over BlockRegion.
Please have a look at the comments 🤓

/**
* @deprecated Use {@link org.terasology.world.chunks.pipeline.stages.ChunkTask} instead
*/
@Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

So we reverted the deprecation? Was the other ChunkTask recently introduced, or has it been around for some time but was never put to "special" use (justifying this deprecation)?

* System which will process light merging after chunk is ready.
*/
@RegisterSystem
public class ChunkLightMergingSystem extends BaseComponentSystem {
Copy link
Member

Choose a reason for hiding this comment

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

Oh, we don't even hook that up to the magic chunk generation off-thread process, but it is just a regular system 🤓 neat!

* Creates ChunkLightMergingSystem which process light merging after chunk is ready for light merging
* Remove LightMerging from ChunkProcessingPipeline
* ChunkProcessingPipeline have only one-to-one steps.
* RelativeSystem should sort chunks to loads itself.
…osition, if processing for ChunkProcessingPipeline.
…eline's methods.

* ChunkProcessingPipeline use Function and Consumer for stage now.
@DarkWeird DarkWeird force-pushed the feature/extract-light-merger-to-postready branch from cb6bbf2 to 72c4129 Compare February 15, 2021 14:49
DarkWeird and others added 5 commits February 15, 2021 17:49
Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
…kImpl.java

Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
…gingSystem.java

Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
…gingSystem.java

Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
@DarkWeird DarkWeird requested a review from skaldarnar February 16, 2021 06:08
@skaldarnar skaldarnar added this to the v4.2.0 milestone Feb 18, 2021
@jdrueckert
Copy link
Member

Tried this out and the chunks seemed to load very slowly... it took quite a while and then a bunch of them popped up... usually it felt like they pop up one by one 🤔

@DarkWeird
Copy link
Contributor Author

DarkWeird commented Feb 20, 2021

Tried this out and the chunks seemed to load very slowly... it took quite a while and then a bunch of them popped up... usually it felt like they pop up one by one thinking

every chunk ready is faster, because lightmerger(a huge time eater) take a lot of time. (also internal chunkprocessing sorting was eliminated and compilcated wait logic was removed too)
then it is only feels. :)
and yeah it looks like load bunch... because RelativesSystem request only bunches of chunks in one tick.
then this chunks loads in executor concurrently..
then bunch of chunks(all or part of chunks) ready onetime and became ready on main thread.

other profit in tests. tests with chunks works faster(and take less memory) now

@pollend
Copy link
Member

pollend commented Feb 22, 2021

@DarkWeird testing this a bit but sometimes restarting the world makes the whole world black.

@DarkWeird
Copy link
Contributor Author

DarkWeird commented Feb 22, 2021

@pollend
Restarting - world purge?

@pollend
Copy link
Member

pollend commented Feb 24, 2021

something isn't quite working. I'm also seeing some very strange lag time between chunks start showing up.

image

@DarkWeird
Copy link
Contributor Author

something isn't quite working. I'm also seeing some very strange lag time between chunks start showing up.

image

Lag is physics's onChunkLoading event handler.

Very strange about black.. maybe you needs to update your reflections cache?

@pollend
Copy link
Member

pollend commented Feb 24, 2021

Some of the chunks are loading in this way and the other half are fine.

@DarkWeird
Copy link
Contributor Author

Some of the chunks are loading in this way and the other half are fine.

No. It is not about some of the chunks.
It bunch of them at once. (Iterate 100+ chunks iterate every chunk per block)
I try load them with limits.. it was worster

* strange, hashset not determinate that two Vector3i has same hashcode, if this vectors not same...
…postready' into feature/extract-light-merger-to-postready
@DarkWeird DarkWeird force-pushed the feature/extract-light-merger-to-postready branch from 9c99f00 to 8c837a1 Compare February 24, 2021 14:42
…mpletableFuture with simple function Composition.

* function composition seems work faster.
@skaldarnar
Copy link
Member

The lag spikes don't seem to be that big anymore (at least I did not notice them that much) but you can still end up with black/unlit areas.

image

This happened after I've waited for the LOD chunks to generate, and then moved into some direction...

@DarkWeird
Copy link
Contributor Author

The lag spikes don't seem to be that big anymore (at least I did not notice them that much) but you can still end up with black/unlit areas.

image

This happened after I've waited for the LOD chunks to generate, and then moved into some direction...

Just wait. Idk why this works so bad.

ChunkLightMergingSystem checks around chunks on every chunk loading for possible to start light merging...
Maybe because working threads have the Lowest priority....

}
chunks[index++] = chunkProvider.getChunk(position);
}
threadManager.submitTask("Chunk Light Merging", () -> new LightMerger().merge(chunks));
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look thread safe. It's changing the light values for a chunk that's already accessible elsewhere, so another system could be updating the lighting at the same time. Also, it looks like you don't mark the chunk dirty anywhere after light merging happens, so it may never be re-meshed (depending on the order of operations).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it not thread safe, but it costs low price if error happens there.
About dirting chunks. It marks implicitly.(somewhere during merging)

Copy link
Contributor

@4Denthusiast 4Denthusiast left a comment

Choose a reason for hiding this comment

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

The changes to the world collision systems seems pretty irrelevant, and it's annoying to have that mixed into the same PR. I can't find any obvious problems that would cause the mostly unlit chunks shown in the screenshots. Internal light merging is clearly still happening, otherwise there wouldn't be that light at the bottom of the chunks, but somehow it's starting off dark, rather than starting off lit at the top like it should be.

I haven't had a look at everything yet. In particular, I haven't read through all of the changes to ChunkProcessingPipeline. @DarkWeird Could you summarise what those changes are meant to do (except directly removing the light merging step)? It looks like a lot has been removed. Does that reduce the functionality of it? If it removes the ability to have steps that require multiple chunks at once, I would expect it could be simplified much more than this.

@keturn keturn modified the milestones: v4.3.0, v4.4.0 Mar 14, 2021
@DarkWeird
Copy link
Contributor Author

DarkWeird commented Mar 14, 2021

@4Denthusiast

  1. Light merging as separate system which works at chunks's onLoaded event
  2. Simplify pipeline with saves interface..(almost)
  3. Changes to physics system because chunks began incoming to it too fast (freezes main thread, because needs to process many chunks at main thread):
    3.1. Direct blockID getter for chunks
    3.2. merge physics voxels systems to one (removes second full chunk scan for block id)
  4. You can load only one chunk if you require instead 27(3x3x3) chunks when needs only one.

@DarkWeird
Copy link
Contributor Author

@4Denthusiast

About unlit.
I don't know how internalLightProcessor works exactly.
I don't change it.
Mostly lightning data at early stage receives from world generator or internalLightProcessor.

Mostly lightning data at all - LightMerger.

Unlit border happens because lightmerger is not processed those chunks. But it cannot process them, because not enough chunks (next chunks is lod chunk already)

@pollend
Copy link
Member

pollend commented Mar 14, 2021

@4Denthusiast

About unlit.
I don't know how internalLightProcessor works exactly.
I don't change it.
Mostly lightning data at early stage receives from world generator or internalLightProcessor.

Mostly lightning data at all - LightMerger.

Unlit border happens because lightmerger is not processed those chunks. But it cannot process them, because not enough chunks (next chunks is lod chunk already)

What are the main issues that make lighting inefficient. just shuffling things around does not resolve the problem does it :?. I see large spikes now with the lighting being moved to the main thread. this should be a parallelizable problem since lighting is local to its chunk. the only thing that would matter is the locations of the active light source and a border similar to how facets work. I also don't see why we can't cache out the lighting to the filesystem to avoid having to recalculate it every time.

@DarkWeird
Copy link
Contributor Author

@pollend
Seems i saw old field in protobuf, which used for light data..

Hmm. I suspect that internalLightProcesdor works incorrect.

Also.. provide facet data to game can helps make lightning better... Quality change

@pollend
Copy link
Member

pollend commented Mar 14, 2021

@pollend
Seems i saw old field in protobuf, which used for light data..

Hmm. I suspect that internalLightProcesdor works incorrect.

Also.. provide facet data to game can helps make lightning better... Quality change

one thing I would like is to have a dynamic light facet for sunlight and just move the light column up the chain until it hits an unloaded chunk and then move the up the next visible block. so we don't have areas underground that are lit because a chunk was momentarily exposed to the sky.

@4Denthusiast
Copy link
Contributor

What are the main issues that make lighting inefficient. just shuffling things around does not resolve the problem does it :?.

The issue that this PR fixes is that you need to load all the chunks around a chunk before it can become visible. That's sort of an efficiency thing in that it reduces the amount of woth necessary before a given amount of chunks become visible.

@4Denthusiast
Copy link
Contributor

I'm not happy with just merging this with the concurrency issues left in. Although I agree that the issues would be mostly minor, and probably rare, they would also likely be really confusing. Others who have actually tested this PR have commented that it causes the game to have noticable lag spikes, and fixing the concurrency issue would make that even worse. Possibly that could be improved by improving the efficiency of the lighting calculations (which Michael thinks is very possible), but I think overall this change may just not be worth making, so I suggest just closing the PR unmerged.

@DarkWeird DarkWeird closed this Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Performance Requests, Issues and Changes targeting performance Topic: Stabilization Requests, Issues and Changes related to improving stablity and reducing flakyness Type: Improvement Request for or addition/enhancement of a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants