search

google / blockly

The web-based visual programming editor.
https://developers.google.com/blockly/
Apache License 2.0
12.51k stars 3.72k forks source link

fix: bump initiator group in an orthogonal direction from neighboring group #8613

Closed johnnesky closed 4 weeks ago

johnnesky commented 1 month ago

The basics

The details

Resolves

Fixes https://github.com/google/blockly/issues/8168

Proposed Changes

When two connections that are compatible but are not connected to each other are close enough to trigger a bump, the direction to bump in now depends on whether the inferior connection is part of the "initiator" group. Previously, if they were both movable, the inferior connection would always be bumped down and right. Now, it could be bumped up and right if the inferior connection is in the initiating group, meaning the group that was manipulated most recently, causing its input neighbors to be checked for bumping.

I also refactored related logic for readability and codified the existing invariant that the parameter to the bump function is the superior connection.

Reason for Changes

It's possible for two independent groups of connected blocks to have multiple neighboring pairs of connections that can bump each other. It's further possible that both groups can have an inferior connection that would be bumped by the other group's connection. https://github.com/google/blockly/issues/8168 demonstrates such a scenario, where the logic_operation block's inferior output connection is bumped by the outer controls_if, which moves it downward and puts its superior input connection near the logic_compare block's output connection, resulting in the other group getting bumped. This all happens instantaneously, and results in both groups of blocks moving down and right, lining both of them up again to get bumped the next time bumping is triggered.

It is important that the two groups get bumped in orthogonal directions to make sure they get separated in the end, but the connection type is not enough information to distinguish the groups of blocks. Instead, I designated one group the initiator, and that group gets moved in a different direction.

Test Coverage

All existing unit tests pass (except for 4 test methods that were already failing due to coordinates being slightly different when testing on my mac). I manually tested that the linked bug is avoided by this PR.

Documentation

N/A

Additional Information

N/A

johnnesky commented 1 month ago

Whoops, I've noticed that the first version of this PR isn't foolproof, and can create situations where both groups of blocks move up and right instead. For example:

https://github.com/user-attachments/assets/19a786e9-c620-4aa3-a5fc-34b67b76aeb6

A better strategy would be to treat connections differently depending on whether they are the "initiating" connection, i.e. they're part of the group that BlockSvg.bumpNeighbours() was called on. That would consistently treat one group of blocks differently than the other. I have updated the PR to use this strategy.

johnnesky commented 1 month ago

I updated the PR, and it seems to be more effective in preventing infinite bump cycles now.