Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

johnnesky
Copy link
Member

@johnnesky johnnesky commented Oct 7, 2024

The basics

The details

Resolves

Fixes #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. #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 johnnesky requested a review from a team as a code owner October 7, 2024 19:05
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Oct 7, 2024
@johnnesky johnnesky changed the title fix: bump connected connections in a different direction fix: bump superior neighbors of connected inferior connections in an orthogonal direction Oct 7, 2024
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Oct 7, 2024
@johnnesky
Copy link
Member Author

johnnesky commented Oct 7, 2024

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:

Screen.Recording.2024-10-07.at.2.13.22.PM.mov

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 johnnesky marked this pull request as draft October 8, 2024 00:14
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Oct 11, 2024
@johnnesky
Copy link
Member Author

johnnesky commented Oct 11, 2024

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

@johnnesky johnnesky marked this pull request as ready for review October 11, 2024 21:37
@johnnesky johnnesky changed the title fix: bump superior neighbors of connected inferior connections in an orthogonal direction fix: bump initiator group in an orthogonal direction from neighboring group Oct 11, 2024
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Oct 11, 2024
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Oct 14, 2024
config.snapRadius -
Math.floor(Math.random() * BUMP_RANDOMNESS) -
this.x;
const selected = common.getSelected() == dynamicRootBlock;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: probably want === here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New dancing blocks with Zelos - blocks move when field edited
2 participants