CaffeineMC / sodium-fabric

A Minecraft mod designed to improve frame rates and reduce micro-stutter
Other
4.7k stars 800 forks source link

Lettings mods disable ClientChunkManagerMixin #1976

Closed noah1510 closed 1 year ago

noah1510 commented 1 year ago

Request Description

I am one of the people working on getting bobby to work with sodium 0.5.0 look here for details. While it basically works there is one rendering problem which causes a lot of flickering. All the flickering is gone when 'mixins.core.world.map.ClientChunkManagerMixin' is dsiabled (at the moment done by building a custom sodium version commenting its functions out).

This is probably the last remaining problem. It would be possible to call ChunkTrackerHolder.get(this.world).onChunkStatusAdded(chunkX, chunkZ, ChunkStatus.FLAG_HAS_BLOCK_DATA); right after ChunkTrackerHolder.get(this.world).onChunkStatusRemoved(chunkX, chunkZ, ChunkStatus.FLAG_HAS_BLOCK_DATA); in a different mixin into the ClientChunkManager. However this could cause weird behavior since the chunk would be added to the add and remove queue.

So an option to disable the mixin seems like the best solution. Do you think there is a way to add this soon? All other solutions seem to be pretty inefficient or cause other potential problems. Thanks in advance

jellysquid3 commented 1 year ago

Let's see what we can make happen... I'll add this compatibility issue to our milestone goal for the next release.

Altirix commented 1 year ago

can add a little to this, the problem stems from vanilla chunks being unloaded and replaced by bobby chunks.

the unloading of vanilla chunks first will remove the block data flag from neigbouring chunks & again for all flags when it gets the unload packet.

currently at least i think it should be possible to mixin to chunkTracker then check if the chunk is now an instance of bobbys fake chunk and cancel the method before the neigbours check the flags of chunks around them (depends if the chunk is replaced before the flags are set, ive not checked)

noah1510 commented 1 year ago

I tried a mixin that overwrites the update neighbor function of the chunk tracker, to only update the chunk itself not it's neighbors. The problem is actually that the flag is removed when it shouldn't be. It looks not as bad when only the unloaded chunks are affected but that does not completely fix it.

Like mentioned before immediately setting the data flag again is also not ideal since the remove function adds the chunk to the unload queue and the set function would add the same chunks to the load queue. Besides being horribly inefficient thir might cause some even more nasty bugs.

Altirix commented 1 year ago

I tried a mixin that overwrites the update neighbor function of the chunk tracker, to only update the chunk itself not it's neighbors. The problem is actually that the flag is removed when it shouldn't be. It looks not as bad when only the unloaded chunks are affected but that does not completely fix it.

Like mentioned before immediately setting the data flag again is also not ideal since the remove function adds the chunk to the unload queue and the set function would add the same chunks to the load queue. Besides being horribly inefficient thir might cause some even more nasty bugs.

thats odd as i had tried essentially the same thing, which worked as expected, tho i wasnt testing that via mixins, i was just building a debug version of sodium (its very possible i never noticed this as my pc was building chunks near instantly.)

ci.cancel wont be resetting the chunk flags, itll return the method early. but ive had a look and acutally rather than ci.cancel before update neigbours. its possible to use the unload packet mixin from SodiumClientPlayNetworkHandlerMixin which ci.cancels before sodiums unload hooks when the chunk is replaced. it sounds like the same should occur here now.

will update on the bobby pr

Johni0702 commented 1 year ago

Like mentioned before immediately setting the data flag again is also not ideal since the remove function adds the chunk to the unload queue and the set function would add the same chunks to the load queue. Besides being horribly inefficient thir might cause some even more nasty bugs.

This is fixed by https://github.com/CaffeineMC/sodium-fabric/pull/2001. And with that, I believe it is now perfectly possible for Bobby to work with ClientChunkManagerMixin by simply calling onChunkStatusAdded for the fake chunk immediately after that mixin unloads the real chunk (and vice versa for unloading a fake one and loading a real one). I've pushed my preliminary attempt at doing so to a temporary branch until I get around to testing it a bit more but from a few simple test it seems to function well.

As such, I believe no further changes are required on Sodium's end.

jellysquid3 commented 1 year ago

I'm going to close this issue seeing as it should be possible to fix the problems without further changes on our side. Please re-open if you are still having issues with compatibility.