World.updateEntityWithOptionalForce has sense of "forceUpdate" reversed. (FWD from MC-119548)

[Pokechu22]

#As brought up by Timothy Miller on mojira in MC-119548, the forceUpdate parameter to "World.updateEntityWithOptionalForce" seems to be reversed.

Timothy Miller writes:

I am in the midst of investigating MC-22147, where some others are testing the proposed fixes. They are still running into some bugs related to processing in lazy chunks, so I've been looking into how that is handled. Specifically, I'm looking at entity processing at this time, and I'm looking at MCP for 1.12-pre1.

Every game tick, entities get processed by calling World.updateEntities(). For every non-player entity, updateEntity is called. That calls "this.updateEntityWithOptionalForce(ent, true);"

Now, the way the comments are written, the forceUpdate argument should FORCE AN UPDATE, even if the entity is not in a lazy chunk. However, there's this code:

public void updateEntityWithOptionalForce(Entity entityIn, boolean forceUpdate)
    {
        // unimportant code not pasted
        if (!forceUpdate || this.isAreaLoaded(i - 32, 0, j - 32, i + 32, 0, j + 32, true))

Entity processing occurs if the location has surrounding chunks loaded (i.e. is not a lazy chunk) OR if forceUpdate is FALSE.

That seems like a mistake. All other comments indicate that forceUpdate should ensure that entity processing occurs regardless of whether or not neighboring chunks are loaded, as does other code inside updateEntityWithOptionalForce ifself. But the sense of forceUpdate in that first if is REVERSED. Notice the "!" there.

Now as a consequence, if it is supposed to be the case that entities ticked by World.updateEntities that are in lazy chunks are not supposed to be updated, then this code path is accidentally getting it right. But the sense of forceUpdate is reversed for everything else, and that method is called in lots of places. So if we fix this, then we need to go to World.updateEntities() and change this:

this.updateEntity(entity2);

To this:

this.updateEntityWithOptionalForce(entity2, false);

I think calls to these two methods should be more thoroughly investigated, and I can look into that. But could I please get a quick comment from one of the devs on this? Is there something about this code that I don't understand?

Thanks.

This seems to have been wrong for a long time. Should probably be corrected, but first I think we should make sure we actually understand this.

[williewillus]

If there's ever a question of 'is this MCP name wrong' vs 'is the vanilla code wrong', I would say it's the first 99.999% of the time

[theosib]

So I'm wracking my brain trying to figure out what this forceUpdate argument means. First, let's rename this argument X so we don't think too much about the name and think more about what it's causing to happen.

Looking at World.updateEntityWithOptionalForce, in the first place where we have "if (forceUpdate && entityIn.addedToChunk)", it's doing this:

if X is true and the the entity being updated was just added to a chunk, then:
(a) If entityIn is riding something (it's a passenger), then call updateRidden on the passenger, which gets the vehicle and first calls onUpdate for the passenger then tells the vehicle to update itself with respect to the passenger.
(b) Otherwise, if there's no riding going on, its just calls onUpdate for entityIn, which does some movement stuff.

Then back in World.updateEntityWithOptionalForce, various coordinate checks are done, and the entity might get moved to another chunk.

Then if X is true and the entity being updated was just added to a chunk, then:
entityIn is assumed to be a vehicle, and its passengers are looped over. For each non-dead passenger that has this vehicle as what it's riding, updateEntity (which has forceUpdate set to true) is called for the passenger.

So X pertains to making sure that passengers and vehicles are synchronized.

Looking at the various places where World.updateEntityWithOptionalForce is called with X being false, they all have to do with moving entities between dimensions and worlds. In this case, with X being false, the all the code that sanity checks coordinates and would move entities between chunks is executed, but none of the passenger/vehicle code is run. This is probably because you can't switch worlds while riding an entity, I guess, so there's no point in running the code? But since we're switching worlds, we have to do the coordinate stuff and add entityIn to some chunk.

X is true when the entity is already in a world and could possibly be a vehicle or passenger.

I'm struggling to decide on a new name for X. When it's true, it means "update riding."

But when it's false, it means "moving between worlds, spawning, dismounting, jumping, or teleporting." In other words, sudden or discontinuous changes in position.

Some names that come to mind for X:

• naturalMotion
• smoothMotion
• continuousMotion
• notTeleporting

If there is any basis upon which "forceUpdate" applies, it doesn't mean "make sure an update happens," which is what the comments lead me to believe. Rather, it seems to mean "update on the basis of applied forces." This brings to mind some other possible names:

• physicsUpdate
• newtonianUpdate
• updateWithForce
• updateFromForce

I think the last two are the best, probably because I got those names from a friend. :)

[Pokechu22]

Hm, I can see that. I personally like physicsUpdate; with that we may want to rename the method as well though.

I'll note that the method is only called with the parameter set to true in updateEntity, which in turn only is called in the entity tick (in updateEntities) and recursively by the method if the parameter is set to true for stacked entities. So it could also be something involving tickEntity or the like.

[kashike]

Where are we at on this issue @Pokechu22?

[Pokechu22]

(contacted theosib elsewhere to check): Still a bit unsure about what the best name is, although we're pretty sure that we understand what the parameter actually does.

[theosib]

I'm pretty sure I'm right about what this signal means. The question comes down to any opinions or policies others have about this.

I suggest the following name changes:

• method: Update EntityWithOptionalRiding
• 2nd argument: updateRiding

I'm leaving it a little vague. Based on the description I wrote above, "updateVehicle" might be another possible name, but I can't remember what happens when this method is called on vehicles that have riders. I would have to spend some time digging through the code to be sure, but if you're okay with this, I'd say go ahead and change the names.

Thanks.

[kashike]

I'm not sure those are the right names:

        if (forceUpdate && entityIn.addedToChunk) {
            ++entityIn.ticksExisted;

            if (entityIn.isRiding()) {
                entityIn.updateRidden();
            } else {
                entityIn.onUpdate();
            }
        }

It will tick (update) entityIn no matter what - it will just do some additional work (dismounting, etc).