search

amadornes / MCMultiPart

A universal multipart API for Modern Minecraft
Other
67 stars 22 forks source link

World.notifyBlockUpdate inaccurately mimics (bad) vanilla behaviour #134

Closed asiekierka closed 6 years ago

asiekierka commented 6 years ago

Only flag values 2 and 4 should be handled in notifyBlockUpdate; flag values 1 and 16 should be handled in markAndNotifyBlock.

2xsaiko commented 6 years ago

Would this be the correct way to handle these methods? https://github.com/therealfarfetchd/MCMultiPart/commit/8c2f076f30d0887afe8d798ff0224e0d1380bd12

asiekierka commented 6 years ago

No. markAndNotifyBlock needs to call all of them, and notifyBlockUpdate only the ones traditionally handled by IWorldEventListeners. getActualWorld() will not backtrack!

2xsaiko commented 6 years ago

Huh? World's markAndNotifyBlock works the same way with it calling notifyBlockUpdate, I copied that if block straight from World. Other than that, vanilla World.markAndNotifyBlock specifically only handles flags 1 and 16 itself. I did notice that the condition for flag 16 was inverted though, that needs to be fixed

Only IWorldEventListener I could find that did anything with the flags was RenderGlobal, and that only handles flag 8, which isn't handled at all in MCMP's code. Should I handle every flag in markAndNotifyBlock then?

asiekierka commented 6 years ago

Huh? World's markAndNotifyBlock works the same way with it calling notifyBlockUpdate, I copied that if block straight from World. Other than that, vanilla World.markAndNotifyBlock specifically only handles flags 1 and 16 itself. I did notice that the condition for flag 16 was inverted though, that needs to be fixed

Yes, but the ActualWorld's markAndNotifyBlock will call the ActualWorld's notifyBlockUpdate, which won't then handle the multipart stuff.

2xsaiko commented 6 years ago

But I'm also calling the wrapped world's notifyBlockUpdate under the same conditions that the actual world's notifyBlockUpdate will get called in markAndNotifyBlock, so the multipart stuff is already handled. Right? I feel like I'm missing something here, haha

asiekierka commented 6 years ago

notifyBlockUpdate(pos, iblockstate, newState, flags);

okay, i'm stupid

HOWEVER! This will cause getActualWorld().notifyBlockUpdate() to be called twice. Not as bad, but could still impact performance a little

asiekierka commented 6 years ago

HOWEVER! This will cause getActualWorld().notifyBlockUpdate() to be called twice. Not as bad, but could still impact performance a little

do not forget

2xsaiko commented 6 years ago

yeah, the last commit should fix that.

asiekierka commented 6 years ago

Aha! Seems like it. Thanks!

amadornes commented 6 years ago

I'll push a release before going to bed :)