Fix reach ring

[DarkShadow44]

Fixes GTNewHorizons/GT-New-Horizons-Modpack#20484

Built on top of #67, hopefully this time fixed for good..

To summarize, the ring has a few issue:
Primarily, it didn't get applied after a relog, that's why it got handled in onWorkTick as well.
Problem is, after unequip we might get another tick, which might reapply the effect erroneously.
The current code deal with that by expecting that tick and handling it.
Additional issue is the shared state of client/server in SP (toRemoveItems)
Since expecting (and needing) the ticks is a pain to deal with, I replaced it with a short delay - those weird ticks after unequip are now ignored and don't execute any logic. Should make it easier to understand and hopefully fix this issue.

@Shadow1590 Since you are the original author, mind taking a look as well?

[Shadow1590]

The code looks fine by me. (But im not gonna review cause im not part of the org)
Although i have to say that all the worntick stuff is there because baubles doesnt have anything that fires only on login/load into world. So if anyone can and wants to do that it would remove the need for at least this bauble to be tickable.

[Dream-Master]

@Shadow1590 i can add you to the org if you can review PRs .

[sisyphussy]

I don't think this is the way you should fix it. You're essentially fixing the issue, but not the root cause. The underlying issue is that the function setReach() gets called twice on the client when equipping the ring (and only once when unequipping).

[Nikolay-Sitnikov]

The code looks fine by me. (But im not gonna review cause im not part of the org) Although i have to say that all the worntick stuff is there because baubles doesnt have anything that fires only on login/load into world. So if anyone can and wants to do that it would remove the need for at least this bauble to be tickable.

There is a PlayerJoinEvent provided by Forge. Why not scan the Baubles inventory whenever a player logs on and activate their Ring?

[Shadow1590]

The code looks fine by me. (But im not gonna review cause im not part of the org) Although i have to say that all the worntick stuff is there because baubles doesnt have anything that fires only on login/load into world. So if anyone can and wants to do that it would remove the need for at least this bauble to be tickable.

There is a PlayerJoinEvent provided by Forge. Why not scan the Baubles inventory whenever a player logs on and activate their Ring?

Sure, but i think that should be part of baubles and not here.
I didnt originally do it because i havent found another bauble that would benefit from it. If there would be more baubles I can make something.

[Dream-Master]

We have a mod Radom baubles for more stuff

[DarkShadow44]

The code looks fine by me. (But im not gonna review cause im not part of the org) Although i have to say that all the worntick stuff is there because baubles doesnt have anything that fires only on login/load into world. So if anyone can and wants to do that it would remove the need for at least this bauble to be tickable.

Yeah I understand.

I don't think this is the way you should fix it. You're essentially fixing the issue, but not the root cause. The underlying issue is that the function setReach() gets called twice on the client when equipping the ring (and only once when unequipping).

Not really. The first underlying issue is that onEquipped isn't called after logging in
The second issue, with the current approach, is that there are onWornTicks after it's unequipped, which confuses the code.

There is a PlayerJoinEvent provided by Forge. Why not scan the Baubles inventory whenever a player logs on and activate their Ring?

Could be an option, I just worked on what we have already. Should we move this to baubles? Should probably not call onEquipped directly though, since other baubles might not expect it twice.

[Nikolay-Sitnikov]

The code looks fine by me. (But im not gonna review cause im not part of the org) Although i have to say that all the worntick stuff is there because baubles doesnt have anything that fires only on login/load into world. So if anyone can and wants to do that it would remove the need for at least this bauble to be tickable.

Yeah I understand.

I don't think this is the way you should fix it. You're essentially fixing the issue, but not the root cause. The underlying issue is that the function setReach() gets called twice on the client when equipping the ring (and only once when unequipping).

Not really. The first underlying issue is that onEquipped isn't called after logging in The second issue, with the current approach, is that there are onWornTicks after it's unequipped, which confuses the code.

There is a PlayerJoinEvent provided by Forge. Why not scan the Baubles inventory whenever a player logs on and activate their Ring?

Could be an option, I just worked on what we have already. Should we move this to baubles? Should probably not call onEquipped directly though, since other baubles might not expect it twice.

Honestly, I'd leave it as-is and just implement the event into Botania's code directly. This removes the worn tick after taking it off problem, and it doesn't require Baubles API to get modified.

If you really want, you could probably add an interface & an event to Baubles, but that would require another PR while most mods wouldn't need it.

[DarkShadow44]

I could use PlayerLoggedInEvent for server side, but that doesn't fire client side. What event do I use to detected that a client has joined a server/SP?

[Shadow1590]

I used FMLNetworkEvent.ClientDisconnectionFromServerEvent for disconnecting. There should be something for connecting

[DarkShadow44]

Thanks, I pushed a new approach that seems to work for me.
I have

• One server side login event
• Two client side events, one to detect when a new server got joined (SP counts) and one to actually apply the change
• A fix for right-clicking the ring to equip it, since it wasn't called client side

[Shadow1590]

I found a problem.
when you log out of singleplayer with a reach ring and then log in to a server where you dont have one equipped it still applies the reach client side (seems like the bauble inventory is not synced yet)
Im also seing two onEquipped calls on client (but that maybe is a baubles thing, because i cant find any call for that in botania)
Im now thinking it should just get synced from server to client via a packet or something

[DarkShadow44]

I found a problem. when you log out of singleplayer with a reach ring and then log in to a server where you dont have one equipped it still applies the reach client side (seems like the bauble inventory is not synced yet) Im also seing two onEquipped calls on client (but that maybe is a baubles thing, because i cant find any call for that in botania) Im now thinking it should just get synced from server to client via a packet or something

Thanks for the report, since syncing should be done by baubles I decided to finally put in a PR to baubles itself: GTNewHorizons/Baubles-Expanded#14

Of course, this doesn't compile now since it references old baubles, but it should be enough for review and testing, I hope.
I think this is the cleanest way now.

[DarkShadow44]

Regarding 5e82cb0, after dying we might have a different player object (a copy?) so we need to compare UUIDs rather than the object itself. Otherwise we will get the unequip for the death (due to server synchronization), but don't apply it because the player is "different" now, keeping out extended reach.