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

Defer Redstone Cache Initialisation to the start of the world tick #1648

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

warjort
Copy link
Contributor

@warjort warjort commented Jun 1, 2021

What:
This is an alternate fix to #1623 which attempts to resolve #1256

I believe #1623 has some potential issues/limitations;

  • The redstone cache is no longer initialised in onLoad() meaning subclasses that depend on this won't work
  • By moving the initialisation to the first tick of the tile entity, it is possible for a block update in the tick of a neighbouring tile to cause the redstone cache to be accessed before it has been initialised (because the tile hasn't yet had its first tick, but its neighbour has).
  • Doing the same thing for pipes is difficult because not all pipes tick
  • The onLoad() is actually called on the client when you place a block. It might cause problems if the redstone cache init is deferred to the block tick, rather than being run immediately?

How solved:
Use a modified version of the existing TaskScheduler, which I have called the FirstTickScheduler.

Changed MetaTileEntityHolder and TileEntityPipeBase to do their real onLoad() via this scheduler.
In particular MetaTileEntity subclasses should not see a change in behaviour for onLoad()

On the server, the scheduler does the processing in the world tick (start phase) rather than the block ticks.
Meaning all blocks get initialised before they do any real work, rather than sporadic initialisation by each block tick.

On the client it just runs the task handler straight away like before.

Outcome:
This should do what #1623 is trying to achieve?

Additional info:
I am going to mark this as draft.
#1256 has been notoriously difficult to reproduce so I don't know if this really fixes it or just moves the problem.
This also needs some proper testing beyond the basic stand up tests I have already done.

@warjort warjort marked this pull request as draft June 1, 2021 21:21
@serenibyss
Copy link
Collaborator

The onLoad() is actually called on the client when you place a block. It might cause problems if the redstone cache init is deferred to the block tick, rather than being run immediately?

I believe that update should be called almost immediately as soon as the block is fully initialized, so I doubt this would have a real issue. To me, this proposed solution seems like more code than it needs to be compared to moving it to a first-tick update check, but like you said its hard to test and see which solution is actually better.

@warjort
Copy link
Contributor Author

warjort commented Jun 4, 2021

To me, this proposed solution seems like more code than it needs to be compared to moving it to a first-tick update check

My argument is that it is needed for the 4 reasons outlined above (there maybe others I haven't thought about).

The mechanism I have used is similar to how tile entities are themselves loaded by vanilla/forge.
There it remembers the skeleton tile entities during initial chunk loading then runs a separate pass to actually initialise them (NBT -> tile entity).

I have just introduced a 2nd pass that moves the onLoad() that is causing problems away from the chunk loading thread to the start of the world tick.
Compared to the block tick solution, this removes some possibilities that the tile entities are accessed before their onLoad() is completed.
e.g. see the processing in TickableWorldPipeNetHandler, but it could be any processing in some random 3rd party mod
And also my point about neighbour block updates.

@warjort
Copy link
Contributor Author

warjort commented Jun 4, 2021

An alternative approach is to re-write the redstone cache so that it works under a lazy initialisation protocol.

This is a much more invasive change, but it would have the additional advantages that it would remove unnecessary work for tiles that never process their redstone signals and distribute the work over time to when it is acutally used (improving chunk loading times).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ConcurrentModificationException causing server crash
2 participants