Adds a test to determine piston stickyness

[b-ncMN]

This commit adds a condition to prevent sticky pistons from pulling items back if they have been extended for <= 2 ticks

Fixes #5365

[bearbin]

Looks reasonable, just want to make sure the semantics here are desired.

Also, please add your name to the CONTRIBUTORS file so your contribution is recognised and to indicate your agreement to the LICENSE terms.

[bearbin]

ExtensionWorldTickTime is not persisted, so when chunks are un- and re-loaded, this will always return when a newly-loaded extended piston is retracted. Is that the correct behaviour?

[b-ncMN]

Yeah, this might be an issue if the player let's a 2 tick clock running and goes to unload and load the chunk

[b-ncMN]

I'm not sure if this is still relevant with the changes I just pushed

[KingCol13]

Since there is only one handler and everything is static wouldn't this mean that if:

Piston 1 extends at tick 0
Piston 2 extends at tick 500
Piston 1 retracts at tick 501

Piston 1 will not retract its connected block as piston 2 has just set ExtensionWorldTickTime?

I think we need to keep a separate ExtensionWorldTickTime for each piston somehow.

[bearbin]

Oh yes, this is a BlockHandler not a BlockEntity! This won't work at all with more than one piston in the world, it needs something to be stored per-piston.

[bearbin]

Possibly in the redstone simulator? Not sure though, this needs a bit of thought.

[Pokechu22]

Pistons normally become a block entity while moving ("block 36"/the moving piston block thing). That block entity knows what block is being pushed and when the push should finish (if I recall correctly; the wiki should document it in more detail).

[bearbin]

We don't have a block entity for pistons at the moment (at least I think so). Creating one and using it to store this value makes sense.

[b-ncMN]

Oh yes, this is a BlockHandler not a BlockEntity! This won't work at all with more than one piston in the world, it needs something to be stored per-piston.

I totally did not thing of that at all

[b-ncMN]

I have created a PistonEntity class to represent both pistons and sticky pistons, but now I'm unsure where to go from there
where do I invoke this constructor that I wrote and how do I get to use it ?

[tigerw]

I have a feeling adding moving piston tile entities is going to be a lot of work (and introduce dupe bugs), we should figure out how vanilla pistons respond to signals and extension, retraction timings before starting any changes.

Questions like how long does it take to extend, what happens when it loses or gains signal, a part of the piston is destroyed or a block is placed in the way during extension, retraction, what happens if the signal is pulsed in the same tick. Ideally there would be some way to replicate vanilla while only relying on the piston blocks as state. If we start adding too much complexity there's the risk of us repeating the countless vanilla piston dupe bugs over the years.

[b-ncMN]

I have a feeling adding moving piston tile entities is going to be a lot of work (and introduce dupe bugs), we should figure out how vanilla pistons respond to signals and extension, retraction timings before starting any changes.

Questions like how long does it take to extend, what happens when it loses or gains signal, a part of the piston is destroyed or a block is placed in the way during extension, retraction, what happens if the signal is pulsed in the same tick. Ideally there would be some way to replicate vanilla while only relying on the piston blocks as state. If we start adding too much complexity there's the risk of us repeating the countless vanilla piston dupe bugs over the years.

But why would we need to add moving piston tile entities in the first place?
Piston entities should be enough, right ? We only need this to define a behavior proper to each instance of piston on a world (to fix the Piston 1 extends Piston 2 extends Piston 1 retracts issue)

[tigerw]

I think Vanilla doesn't have piston entities, only the moving piston entities.

[b-ncMN]

I think Vanilla doesn't have piston entities, only the moving piston entities.

So we MUST have it too ? I think that as long as we can keep away from (possible uselessly) added complexity, it's better, no?

[tigerw]

nah I don't like moving piston entities, hopefully we can figure something out which doesn't need any block entities at all. First though, we should have a spec of how pistons should behave on a tick-by-tick level, but I can't find this information on the wiki.

[b-ncMN]

nah I don't like moving piston entities, hopefully we can figure something out which doesn't need any block entities at all. First though, we should have a spec of how pistons should behave on a tick-by-tick level, but I can't find this information on the wiki.

Do you mind elaborating ? I'm not quite sure I understood everything ._. (and why we need to see why pistons work on a tick-by-tick level)

[tigerw]

At the moment Cuberite pistons work like this.

From a resting state,

Tick 0:

• If received redstone power
  • Check if is retracted
  • If yes, broadcast an extension block action & queue an extension task 1 tick later
• If lost power
  • Check if is extended
  • If yes, broadcast a retraction block action & queue a retraction task 1 tick later
• If power unchanged, do nothing

Tick 1:

• If received redstone power
  • Check if is retracted
  • If yes, broadcast an extension block action & queue an extension task 1 tick later
• If lost power
  • Check if is extended
  • If yes, broadcast a retraction block action & queue a retraction task 1 tick later
• If power unchanged, do nothing
• If an extension task fired
  • Check if can extend
  • If so, set the entire state atomically, including piston base, arm, and pushed blocks
• If a retraction task fired
  • Check if can retract
  • If so, set the entire state atomically, including piston base, arm, and pulled block

and tick 2, 3 repeats.

From this we can see that how a piston behaves depends on the state of the world (GetBlock on the piston position and redstone power), and scheduled tasks which call into the piston handler.

We also see why pistons don't drop blocks as vanilla does, because the state of the pulled block is updated atomically, the block is either pulled or not; there are no intermediate states visible to the outside.

[Pokechu22]

a part of the piston is destroyed or a block is placed in the way during extension

I believe the answer here is that the moving piston block entity is considered indestructible. At least, it's impossible to place a block in the way during extension because the destination block is also converted into a moving piston block (with its block entity) during the moving process, and you can't place something into that. Breaking I'm less sure of; you can't mine it, but I'm not sure what happens if it's destroyed via an explosion.

[tigerw]

If we want to replicated vanilla behaviour, it seems we have to both

• extend the piston extension/retraction time from 1 tick to 3, and
• potentially split the atomic update of the blocks into separate updates of the piston base, arm, and pushed/pulled block

But, to do this robustly, I think we need to know exactly what vanilla does each tick a piston is active and how it responds to certain events in these newly introduced intermediate states, and formulate some sort of system to imitate it. As an example, why does vanilla take 3 ticks to extend a piston? How is it scheduling the events? Is it 1 tick delay + 2 ticks actual animation? Or something else?

The system we eventually come up with will ideally not need moving piston block entities, because that's extra work to save and load to disk, and unclear is if those entities need to be sent to the client, and if doing that would conflict with the piston animation the client plays (which also using moving piston entities). But if that's the only way, oh well. However a piston entity wouldn't work because we'd diverge from what vanilla does - the save file could have our own custom block entity in it - and ideally Cuberite tries to remain compatible.

[tigerw]

I don't have the answers to this, maybe Pokechu our resident expert will, but these are only relevant if you want to overhaul pistons to make them 100% vanilla identical.

---

If ExtensionWorldTickTime is indeed a quick fix for this issue and you only need it to be instanced per-piston, you could consider adding a map in cIncrementalRedstoneSimulatorChunkData which will store these times per block coordinate.

[tigerw]

(as bearbin has already suggested at the start)

[b-ncMN]

However a piston entity wouldn't work because we'd diverge from what vanilla does - the save file could have our own custom block entity in it - and ideally Cuberite tries to remain compatible.

Right, this is where it causes problems at, this means that the world wouldn't be able to be used by regular minecraft-server

[Pokechu22]

and unclear is if those entities need to be sent to the client

I don't think you need to send anything special to the client other than the piston block action, but unfortunately that's basically all I know on the subject; I don't have any further details on hand (and things may have changed since that rather sparse documentation was written, too).

[b-ncMN]

I don't have the answers to this, maybe Pokechu our resident expert will, but these are only relevant if you want to overhaul pistons to make them 100% vanilla identical.

I mean, if not doing so could cause some major issues, then I see no reason why I would want to implement a temporary fix, I might as well try to overhaul the piston system (although I think I'm far away from having the technical knowledge to do such a thing)
In such a case of an overhaul, as KingCol13 suggested (on Discord), it might be a smart idea to put up a list of things to do

[Pokechu22]

Actually, I remembered something else. I think the only relevant property of the moving piston/block 36 block entity for servers is that when the progress field hits its maximum, the block changes to the stored block. Everything else is only relevant clientside. Here's an old video showcasing an unusual use for block 36 (this predates command blocks); I think there was a commentated video that gave more context but I can't find it (the video credits Minecraft Forum user DiEvAl though, so maybe it's somewhere in their posts).

[Pokechu22]

I managed to dig up further context: this video (which is no longer public, but is archived here (invisible on the main page due to dependence on YouTube's flash player, I think, but also archive.org's download speed is rather slow and you're best off saving that video manually (~10 minutes), waiting for the download to finish, and then viewing it locally)). This video is from late 2012 (9 years old, yikes!), but should be somewhat vaguely informative. Note that in that case, progress is set to a negative value; from that I gather that normally progress starts at 0.0 and ends at 1.0.

[b-ncMN]

So uh, I'm not sure where I would start of with something like this.. Trying to put together a list of things that have to be done for that overhaul is gonna be a challenge in itself

[b-ncMN]

I have decided to revert everything, as a plan to avoid using an entity altogether

[KingCol13]

Suggested change

	std::unordered_map<Vector3i, cTickTimeLong> PistonExtensionWorldTickTime;
	std::unordered_map<Vector3i, cTickTimeLong, VectorHasher<int>> PistonExtensionWorldTickTime;

Need the hasher to fix compile.

[KingCol13]

Should this be uncommented?

[KingCol13]

also this block should have an extra indent