legoatoom / ConnectibleChains

Connect your fences with a decorative chain!
GNU Lesser General Public License v3.0
14 stars 6 forks source link

Duplicate / Overlapping connections #6

Closed Qendolin closed 3 years ago

Qendolin commented 3 years ago

The same two fences or walls can be connected multiple times which leads to multiple issues.

  1. When a fence is broken or shears are used only one chain item drops.
  2. The chain collision entities remain.

How to reproduce:

  1. Place two fences, A and B.
  2. Connect A to B.
  3. Repeat step 2 at least once.
  4. Break the chain by destroying either A or B or using shears on either A or B.

Note that the issue does not happen when one connection is formed from A to B and another one from B to A.

Environment: Minecraft 1.17.1 Connectible Chains 1.2.2

ps: connect A ble would be the correct word (not connect I ble)

legoatoom commented 3 years ago

I will try to look at when I have the time and energy. Both connectible and connectable are correct.

Qendolin commented 3 years ago

I'll try to look into it myself and open a PR but idk if I have time. (sure both a fine but -able is 'more' correct)

Qendolin commented 3 years ago

The fix is quite easy so I won't make a PR. In ChainKnotEntity.attachChain change the return type to boolean and add these two checks to prevent overlapping connections:

// Check if we have a connection to them
if(this.holdingEntities.containsKey(entity.getId())) return false;
// Check if they have a connection to us
if(entity instanceof ChainKnotEntity knot && knot.holdingEntities.containsKey(this.getId())) {
    return false;
}

Also return true at the end of the function. Then in ConnectibleChains:66 change

} else {
    knot.attachChain(player, true, 0);
    knot.onPlace();

to

} else if(knot.attachChain(player, true, 0)) {
    knot.onPlace();

With this simple fix nothing will happen when trying to connect to a fence where a connection already exists.

Also in ChainKnotEntity you could add this check at the start of createCollision but it's not required

if(COLLISION_STORAGE.containsKey(entity.getId())) return;

PS: Not tested in multiplayer