TE init method w/ world access

[HashtagShell]

Original post:

Spoiler

I have a tile, and I want to do some things when it is loaded (which includes when it is created). There is the TileEntity::onLoad method, but one cannot read the world from it, since, because the world hasn't ben fully loaded yet, it would cause another loading of the world, and ultimately end in a StackOverflowException.

Is there a method that gets called once everything is done loading, so I can do my things? (Dynamic initialization of vars based on the surrounding world)

 

I saw that TEChest does its checking in onTick(), using a "haveIAlreadyIntitialized" boolean, which is what onLoad tells us not to do in its javadoc.

 

2

 

State: Partially Solved

 

Problem 1: Add a TE#onLoaded() method that gets called once, but after this chunk has been fully loaded and made accessible

Solution "reiterate":

• We will adopt the "phases" initialization procedure (already used in mod initialization, preInit, init, postInit...) to ensure that the current chunk is fully available when we run TE#onLoaded().
• This way, we prevent any recursive loads ending with StackOverflows.
• Concretely, we will add calls to all current chunk's TEs' onLoaded() method in Chunk#onChunkLoad(), after the TEs and Entities of the chunk have been initialized and added to the world.
• The event ChunkEvent.Load is posted from where we would like to place our 'hook', so we need to decide whether to put it before or after the event, or split the event into parts (ie. Pre and Post).
• Probably the better alternative is to place the event after our hook, so that the event may influence the outcome of the hook.
• This is inconsistent with the current standard behaviour (if-initialized-in-update) since event handlers may expect world-dependent init to happen as late as update(), ie. after their handler.
  • I don't think this is too big of a discrepancy (the probability that some handler would require an event-then-onLoaded order is very low) to be considered a breaking change.
• All in all, we would insert the hook before L175 of the current patch to Chunk.java, add the onLoaded() method to TE, and update javadoc references in favour of this method.
• This change is (except for the one point) not breaking, currently used techniques will still be working, although discouraged.

 

Problem 2: Come up with a better name for TE#onLoaded()

• This name is not very well conveying its functionality, it is kind of ambiguous

 

Problem 3:  Manage chunks loaded by cascading queries into unloaded chunks more efficiently

• Vanilla just loads all the chunks without caring about anything (not optimal) and then unloads them in batches at intervals
• Forge introduces dormant chunks (better)
• The patch in Problem 1 does not introduce cascading behaviour, but it's related, so we might as well have a go at it.
• I am kind of questioning if we should really attempt to "fix" this problem since we would be changing vanilla mechanics (vanilla chunkloaders)

Edited April 4, 2017 by HashtagShell
Make this thing into a Trello board

[shadowfacts]

It's been known for over a year and isn't going to be fixed, you still have to use the first update tick.

[HashtagShell]

#Sad.

 

Wouldn't calling onLoad in a second iteration, or have another onLoad method (maybe onLoaded?) called, work?

 

1. Load Chunk
   1. Do stuff I don't care about, block loading, etc.
   2. Load tiles
      1. Construct tiles
      2. TE#onLoad()
   3. Do more stuff I don't care about to finish chunk loading
   4. Declare chunk as loaded, make it validly accessible in the World object
   5. Call TE#onLoaded in all tiles in that chunk
2. Continue with stuff

[TheMasterGabriel]

If you were to implement something like that, you would need to ensure that the TE's world access is restricted to the chunk it is in. If you were to allow it to access block positions arbitrarily, then a modder could really easily overflow into an unloaded chunk, which would undoubtedly cause tons of problems.

 

Edit: Also, did you try looking at ChunkEvent.Load? I didn't look into the load order that extensively, so you might run into the same problem. If not, however, you can probably use it, along with a list of TEs to be ticked, to get something that would work similar to your TE#onLoaded.

Edited April 2, 2017 by TheMasterGabriel

[HashtagShell]

Can one not easily overflow into an unloaded chunk in update() as well, also causing a lot of problems?

 

If one could use an event to tick a list of TEs with the chunk already being fully loaded, the place where the event gets called from would be a valid candidate for a forge-added onLoaded method, since this is a functionality I could see many people using.

 

I currently don't have much time, with exams coming up, so I haven't done a lot of research, apart from the fact that TE#onLoad is largely useless for any real use-case.

[quadraxis]

You can overflow into unloaded chunks in update() but those chunks won't get ticked, preventing recursive loads.

 

ChunkEvent.Load is fired from the end of Chunk#onChunkLoad(), and is a 'safe' place to access the world from.

onChunkLoad() could be a viable place for calling another TE method, after World#addTileEntities() is called.

[HashtagShell]

24 minutes ago, quadraxis said:

You can overflow into unloaded chunks in update() but those chunks won't get ticked, preventing recursive loads.

 

Would the same stay true for unloaded chunk accesses at where ChunkEvent.Load is posted from?

[quadraxis]

When a chunk is loaded, onChunkLoad() gets called. If something happens then to cause another chunk to be loaded, onChunkLoad() will get called on that chunk as well. That is the potential issue. update() is only called on chunks that are ticked, so a chunk load there won't necessarily cause a call to update() on the new chunk.

[HashtagShell]

That makes sense.

 

However, I fail to see any problems with recursive calling:

 

1. Chunk A starts loading
2. TE#onLoad() in chunk A   // NO world access allowed
3. Chunk A finishes loading, is fully available
4. TE#onLoaded() in chunk A // World access allowed
   1. Some TE references unloaded chunk B
   2. Chunk B starts loading
   3. TE#onLoad() in chunk B // NO world access allowed, no recursion possible
   4. Chunk B finishes loading, is fully available
   5. TE#onLoaded() in chunk B // World access allowed, let's try to cause recursion
      1. Some TE references loaded chunk A
      2. Chunk A is already loaded, query completes without recursion
   6. All tiles in chunk B have had TE#onLoaded() called on them
   7. Chunk B is now fully initialized
   8. Chunk B is now loaded and initialized, query completes
5. All tiles in chunk A have had TE#onLoaded() called on them
6. Chunk A is now fully initialized

[quadraxis]

The issue isn't infinite recursion, it's just loading cascading to a large number of chunks. A TE in chunk A causes chunk B to load, which causes another TE to load chunk C, etc.

 

Forge flags up when something similar happens during chunk population (see https://github.com/MinecraftForge/MinecraftForge/pull/3756) as it has been noted to cause issues there.

Here the issue might not be so bad, it depends on what block positions the tile entities check - if they only check individual neighbouring blocks, for example, it's not likely to trigger very many spurious chunk loads.

[HashtagShell]

If a chunk is unloaded, and it needs to temp-loaded for computational purposes (ie. it will not be ticked), then skip the TE#onLoaded calls. This might seem like a weird thing to do, but it should fix most of the things while not breaking backwards compatibility and currently expected behaviour.

It's late here, I'll elaborate later.

[TheMasterGabriel]

That would probably hog resources, as you might unload/reload chunks several times. If they didn't unload, that would probably be worse, as your computer would be keeping multiple chunks loaded for 1 block in a completely different area. A solution might be to, if a world operation cascades into another chunk, stop and queue it for when that chunk loads. That way you wouldn't run into cascading chunk gen/loading, but rather have operations waiting to take place. I'm not sure about the feasibility of that, as I can already think of some instances where that wouldn't be ideal (like if you TE needs to check blockstates rather than set them), but I'm just throwing ideas out there for what could be a really useful PR.

Edited April 3, 2017 by TheMasterGabriel

[HashtagShell]

Why are we even calling cascading chunk loading a problem, when vanilla just lets it happen?

 

 

[HashtagShell]

Updated original post with what we have right now and what we are still looking for.

I'll be summing up the mentioned solutions for Problem 3 later, out of time rn.

[quadraxis]

Is there even any need to have two methods? I feel like it would be better just to relocate the existing call to onLoad() to a more suitable place.

[HashtagShell]

I was thinking about that, but two methods means two stages of loading, so it allows more interoperation between TEs.

It's kinda like the loading stages, where you first do blocks/items, and only in init do you do recipes. That way, you can expect the processing in the earlier stage to be done, and build on the data generated by it in the second stage.

Just looping twice at the call site would probably make more sense for this, though, since both calls would have world access.

But limiting world access in the first stage is also not a bad idea, it gives the modder the opportunity to do stuff with their TE when it is loaded, but before other modders could interact with it.

Still, might be a little too overcomplicated loading system for something like TEs.

 

Would be great if people with more experience than me could express their opinion about the possible use-cases of this and if it would help in any way.

[HashtagShell]

I was thinking about Problem 3, and something along the lines of this came to mind:

 

During execution of the TE#onLoaded() method, we replace the TE's world field with a singleton instance of a delegate class to World, let's call that class WorldDelegateDormantLoad for now.

This class will be constructed lazily for each world (maybe even stored inside the world object itself), and it will have the following properties:

• It will store the delegate world in one of its fields.
• The getChunk() (or similar) method will be modified. If the chunk is loaded, return it. If the queried chunk's coords are unloaded, then the chunk will be loaded into a Chunk object from disk, all Entities will be initialized, all TEs will have their TE#onLoad() (if there is one) run, but NOT TE#onLoaded() (this is the part that helps with cascading chunk loading), but its objects will not be added to the world, and it will be stored as a dormant chunk in forge's ChunkManager. If the chunk is dormant (includes the case when it was just dormant-loaded), then return it.
• All methods that reference the world's storage structures (loaded Entities, TEs) will be modified: If the requested position is loaded, then delegate the call to its delegate world. Otherwise, dormant-load it with getChunk() and query the TE/Entity/Block directly from the chunk object, and return it.

Once the TE is done with its TE#onLoaded() logic, we replace the world with the original again. We repeat for all TEs in the chunk that is being loaded.

 

We will also have to modify forge's dormant chunk manager to allow us to store chunks there even though the config might set it to zero. Alternatively, if the config disallows dormant chunks, we could just skip this patch and execute all cascading loads.

 

If a chunk is loaded from the dormant cache by a regular world's loadChunk() call, we will add the chunk's objects to the world and execute all TE#onLoaded() methods prior to returning it for further processing. If a completely unloaded chunk is loaded by such a call, we let the current logic load it the standard way (with exception to doing the delegate thing with all TEs).

 

This relies on the assumption that forge manages dormant chunks in an intelligent manner.

 

If I understand the code correctly, dormant chunks are just chunk objects in memory with their members removed from the world, and they are prevented from ticking, which is ideal for this.

 

This essentially attempts to do a sort of temp-load to the chunk

On 3. 4. 2017 at 0:45 AM, HashtagShell said:

If a chunk is unloaded, and it needs to temp-loaded for computational purposes (ie. it will not be ticked), then skip the TE#onLoaded calls. This might seem like a weird thing to do, but it should fix most of the things while not breaking backwards compatibility and currently expected behaviour.

It's late here, I'll elaborate later.

 

without reloading chunks several times, which was the major caveat I think

On 3. 4. 2017 at 2:37 AM, TheMasterGabriel said:

That would probably hog resources, as you might unload/reload chunks several times. If they didn't unload, that would probably be worse, as your computer would be keeping multiple chunks loaded for 1 block in a completely different area. A solution might be to, if a world operation cascades into another chunk, stop and queue it for when that chunk loads. That way you wouldn't run into cascading chunk gen/loading, but rather have operations waiting to take place. I'm not sure about the feasibility of that, as I can already think of some instances where that wouldn't be ideal (like if you TE needs to check blockstates rather than set them), but I'm just throwing ideas out there for what could be a really useful PR.

3

 

 

 

Queueing the operations is not a bad idea either, but there would have to be some sort of threshold since, without one, you could actually stand in a chunk that is still waiting for a cascaded call in a chunk which is currently unloaded. The best example in practice is a line of chunks, requesting data from the next one in line, longer than renderDistanceRadius.