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

Sometimes when in 'connection mode' the ports are not attachable and instead I can move nodes with my mouse #6850

Closed
radeusgd opened this issue May 25, 2023 · 10 comments · Fixed by #7177
Assignees
Labels
--bug Type: bug --regression Important: regression p-highest Should be completed ASAP

Comments

@radeusgd
Copy link
Member

Let me explain with a video:

connection.maker.not.reacting.to.ports.mp4

When I drag a node output port my mouse is in 'connection mode' - I see a line from the node to the cursor. Normally, I can then attach that line to an input port and create a connection.

But as you can see on the video, this does not work - the ports do not highlight and instead I can actually move the nodes around with the cursor - as if it was in 'normal mode'. The expected behaviour is the usual one - the ports will 'accept' such an incoming connection and I should not be able to interact moving nodes around when in this 'mode'.

I'm not sure if this is a known issue but searching 'ports' did not yield anything.

@radeusgd
Copy link
Member Author

I seem to be encountering this relatively often.

It looks like a potential regression, because I don't remember seeing this issue happen so often in the past.

@wdanilo
Copy link
Member

wdanilo commented May 26, 2023

This looks like a serious regression. CC @Frizi

@farmaazon
Copy link
Contributor

Reproduced. When you picked something in drop down (what have arguments) and then try to connect to this argument, it happens. You can connect to it right after opening project (without touching drop-down), and sometimes editing node in CB enables it as well.

@farmaazon farmaazon added p-highest Should be completed ASAP --regression Important: regression and removed triage labels May 26, 2023
@Frizi Frizi moved this from ❓New to 🔧 Implementation in Issues Board May 26, 2023
@Frizi
Copy link
Contributor

Frizi commented May 26, 2023

I'm able to reproduce it as well, thanks @farmaazon for the steps. looks like the issue is not really about the specific ports being not attachable, but the whole node somehow not recognizing that it is being hovered after a value is selected from a dropdown. This state tends to fix itself after attempting the hover a few times. This behavior is very clearly visible when a debug mode is enabled. I'm going to investigate it further.

@Frizi
Copy link
Contributor

Frizi commented May 26, 2023

I now know the root cause of this, which I would really consider a bug of the mouse hover events. We are using following pattern now to track whether a node or one of its widgets is hovered:

let background_enter = model.background.on_event::<mouse::Enter>();
let background_leave = model.background.on_event::<mouse::Leave>();
background_hover <- bool(&background_leave, &background_enter);

let input_enter = model.input.on_event::<mouse::Over>();
let input_leave = model.input.on_event::<mouse::Out>();
input_hover <- bool(&input_leave, &input_enter);

node_hover <- background_hover || input_hover;
node_hover <- node_hover.debounce().on_change();

This pattern is common in other places of the codebase. There is an assumption that for each Enter event, there will eventually be a Leave event as well. Same for Over and Out. Unfortunately, this is not actually the case. When an element is removed while it is hovered, it will never receive mouse::Leave or mouse::Out events. Therefore the above logic will be blind to that, and node_hover will incorrectly stay true for way too long, and in this case causes the hover areas to never reappear.

I tried various ways of addressing this, and found a few that surely won't work. I believe that what we actually need is a system very similar to focus handling, where the hover state is tracked by the object hierarchy itself. Otherwise it will not be possible to guarantee that event parity invariant, and properly emit events in case currently hovered element changes its parent.

Additionally, the Enter and Leave events are actually not implemented properly on their own, at least if what we want is similar behavior to DOM events. Currently, Enter will only fire if a given object is hovered directly. When hover moves to one of its children, the parent will incorrectly receive a Leave event. This is why I have used Over and Out in the above code in the first place. If we matched DOM behavior, I would be able to use a single pair of Enter and Leave handlers directly on node's main display_object. By implementing the hover handling on the object hierarchy itself, we could easily fix this behavior.

@farmaazon
Copy link
Contributor

When an element is removed while it is hovered

(from the second paragraph)

Do you mean "removed from display object tree", or "dropped"? If the former, cannot we just emit "Out" event when one is removed from its parent? If the latter, then I had a similar problem, and resolved it by "garbage collector" which role is in fact "keep something alive a few frames, so it realizes is no longer on the scene and update states accordingly" - in this case I used deprecated mouse events from shape and, thanks to delaying actual dropping, the FRP network emitted proper "out" event after removing the shape.

Also, cc @wdanilo as we touch EnsoGL internals here.

@Frizi
Copy link
Contributor

Frizi commented May 29, 2023

The issue is that any modification within the display hierarchy can disrupt the parent chain of currently hovered object. Because we rely on those events being propagated up the tree (e.g. in the above case, we listen on model.input object, while events are happening deeper within the widget tree), we have to also maintain guarantees about the events being in pairs at each of those levels, at all times.

So, to be very specific:

  • there is a display object that is currently considered "directly hovered" - i.e. its ID was read from the object ID buffer under the mouse pointer
  • all objects within its parent chain (i.e. direct parent, its parent etc. up to the root) can be considered "transitively hovered"
  • whenever any of the "transitively hovered" is either dropped or is removed from its parent, it must guarantee that corresponding Out and Leave events are sent on its previous parent. The now "orphaned hierarchy" can still contain transitively hovered objects, but nothing else does.
  • Whenever this "orphaned" hierarchy" is again inserted somewhere in the tree, the appropriate Enter and Over events must be propagated from its new parent. Now the new complete parent chain is considered "transitively hovered".

Current hover implementation only sends mouse events at most once per frame, only considered the object hierarchy right after the rendering. It basically lacks the concept of "transitively hovered" objects. All it does is sends the object once from the "directly hovered" object.

scene_was_dirty |= self.layers.update();
scene_was_dirty |= self.update_shape();
context.profiler.measure_data_upload(|| {
scene_was_dirty |= self.update_symbols();
});
self.handle_mouse_over_and_out_events();

We can keep the detection of "directly hovered" object that way, but the logic of maintaining "transitively hovered" object state and actually emitting appropriate events needs to be added into the display object hierarchy itself. This is a similar idea to how we are currently tracking the "focus" state.

cannot we just emit "Out" event when one is removed from its parent?

Yes, but that "just" is doing a lot of heavy lifting here. The objects don't track their hover state yet, and once they are already removed from the parent, it is too late to send any events. There is also no way to detect that a non-direct parent is about to be disconnected and an action is needed.

If the latter, then I had a similar problem, and resolved it by "garbage collector" which role is in fact "keep something alive a few frames, so it realizes is no longer on the scene and update states accordingly"

At the time the objects are already in garbage collector, their parent hierarchy is destroyed and emitted events will not reach any of the old parents. I actually tried a somewhat similar approach (by implementing Drop) and found out about this issue. Once objects are actually being destroyed, it is already too late to act.

@wdanilo
Copy link
Member

wdanilo commented May 30, 2023

@Frizi, @farmaazon, let's have a call tomorrow about it - what do you think of it? This discussion is getting so long, that probably a call will be a lot faster than continuing it here. If it works for you, @Frizi could you schedule it afternoon please?

@sylwiabr sylwiabr added this to the Design Partners milestone Jun 1, 2023
@vitvakatu vitvakatu moved this from 🔧 Implementation to 📤 Backlog in Issues Board Jun 5, 2023
@Frizi
Copy link
Contributor

Frizi commented Jun 5, 2023

We discussed the issue with hover handling on a call. The conclusions:

DOM has similar issues with interaction between mouse events and elements being destroyed, but we agree that we can do better than that. We want to implement better handling of hover states, but doing it eagerly on hierarchy change (similarily to focus) will introduce unacceptable issues:

  • When a hovered object changes its parent, and in effect will be moved away from mouse cursor, its new parent hierarchy would momentarily be considered hovered (it would receive Enter event, and then a Leave event a frame or more later).
  • When any hovered element is momentarily removed from its parent, then added back in, the whole parent chain would receive a "pulse" of Leave and Enter events, even though the user didn't perform any action and there is no other visible change on screen.

To avoid those issues, we want to implement mouse event handling as follows:

  • We want to keep emitting mouse events only from within handle_mouse_over_and_out_events, at most once per frame.
  • Instead of storing and comparing just hovered object ID, we need to maintain the "parent chain" of hovered element.
  • Whenever that chain has changed since last update (either by changes in hierarchy or by a change of hovered object ID), the appropriate events (Enter, Leave, Over, Out) needs to be dispatched to objects in both old and new parent chain, even if old objects are no longer in the same hierarchy.
  • We can make Enter and Leave have the same semantics as in DOM - all parents of hovered elements will receive Enter event when any of their children is hovered.
  • When a new hovered element is detected, but it has a common parent with previously hovered element, their common parent chain will not receive any extra Enter or Leave events.

This gives us a few useful properties:

  • When a hovered object is removed, its parents will automatically receive appropriate Leave and Out events based on above logic, without any additional processing having to happen on object drop.
  • For every received Enter or Over event, the display object will receive exactly one Leave and Out event, as long as it still exists when it is appropriate to dispatch them.
  • Instead of having to manually propagate "X was hovered" through the component FRP endpoints, we can directly attach Enter and Leave event handlers on a display object that wraps given component.
  • It is likely that after implementing this hover logic, we will no longer need "garbage collection" concept in ensogl, and will be able to remove it (though that might need to happen after removal of deprecated mouse events API).

@farmaazon farmaazon removed their assignment Jun 16, 2023
@farmaazon farmaazon added p-high Should be completed in the next sprint and removed p-highest Should be completed ASAP labels Jun 22, 2023
@radeusgd
Copy link
Member Author

Recently, I've been encountering this bug daily, making it really hard to create connections - almost every other time when I try to connect one existing node to another I have to do a rollercoaster of mouse movements around various nodes before the mouse will actually 'stick' to the input port I'm aiming for.

@farmaazon farmaazon added p-highest Should be completed ASAP and removed p-high Should be completed in the next sprint labels Jun 29, 2023
@farmaazon farmaazon assigned farmaazon and unassigned Frizi Jun 29, 2023
@farmaazon farmaazon moved this from 📤 Backlog to 🔧 Implementation in Issues Board Jun 29, 2023
@farmaazon farmaazon mentioned this issue Jun 30, 2023
5 tasks
@farmaazon farmaazon moved this from 🔧 Implementation to 👁️ Code review in Issues Board Jul 3, 2023
@farmaazon farmaazon moved this from 👁️ Code review to 🌟 Q/A review in Issues Board Jul 4, 2023
@github-project-automation github-project-automation bot moved this from 🌟 Q/A review to 🟢 Accepted in Issues Board Jul 6, 2023
@farmaazon farmaazon moved this from 🟢 Accepted to 🗄️ Archived in Issues Board Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug --regression Important: regression p-highest Should be completed ASAP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants