Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

Update to egui 0.21 & fix the interaction bug #85

Merged
merged 3 commits into from
May 9, 2023

Conversation

hakolao
Copy link
Contributor

@hakolao hakolao commented Apr 23, 2023

Trying a fix here for #79

So, I'm pretty sure emilk/egui#2244 broke the movement etc. Because the hover interaction never takes place on the responses for areas covered by other widgets. You can see this by debugging the overlapping widgets with styles. Maybe there is some way to tweak the layers / order of widgets, but dunno. So the solution:

  • change the order of allocate_rect for the background. Before the nodes.
  • And to fix the nodes themselves, call ui.interact before adding the contets.

Perhaps there is a better fix, and I'm not sure if this breaks the dynamic rect size of the widgets, but the example works.

This could at least point someone more knowledgeable towards a better direction so we can update to newer egui.

@kamirr
Copy link
Contributor

kamirr commented Apr 23, 2023

Thanks for the PR, I've tested it against the project I'm working on.

Perhaps there is a better fix, and I'm not sure if this breaks the dynamic rect size of the widgets, but the example works.

That is correct, after resizing a node you can still grab only grab it by the rectangle it was created with. Otherwise everything I've needed seems to work fine:

  • tracking creation / deletion of nodes and connections -- I need to keep another graph-like data structure in sync and even after prolonged use no mismatch has arisen.
  • changing the number of inputs.
  • interacting with both value widgets and bottom ui.
  • persistence.

I needed some egui 0.21 features and this allowed me to drop my fork with them backported, so I'll be working off this PR for the time being. If I find any issues I'll leave a comment.

@hakolao
Copy link
Contributor Author

hakolao commented Apr 23, 2023

Great @kamirr , if you can figure out a fix for the rect bounds, that would be great :)

@kamirr
Copy link
Contributor

kamirr commented Apr 23, 2023

This patch does the trick: 0001-Update-interactable-node-area.txt (note that this is a .patch file as generated by git format-patch, the .txt extension is due to GitHub's attachment policy.)

Feel free to add it to this PR if you like it. tl;dr it stores the outer rect in memory after populating child_ui and then uses this value during the next frame.


let max_height = ui.input().screen_rect.height() * 0.5;
let max_height = ui.input(|i| i.screen_rect.height());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the * 0.5 dropped intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accidentally!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@hakolao
Copy link
Contributor Author

hakolao commented Apr 23, 2023

I'll add those fixes

@hakolao
Copy link
Contributor Author

hakolao commented Apr 23, 2023

Had no clue how to use the patch..., copied it... Thanks for the fix!

@kamirr
Copy link
Contributor

kamirr commented Apr 26, 2023

One thing I’ve noticed is that after some use my program enters a state in which I cannot edit values in DragValues in nodes by keyboard (dragging works fine) and it’s persistent across restarts of the program, until I drop the saved state.

Not yet sure if it’s a bug in egui, this PR or my shenanigans. Will update.

@kamirr
Copy link
Contributor

kamirr commented Apr 26, 2023

One thing I’ve noticed is that after some use my program enters a state in which I cannot edit values in DragValues in nodes by keyboard (dragging works fine) and it’s persistent across restarts of the program, until I drop the saved state.

Not yet sure if it’s a bug in egui, this PR or my shenanigans. Will update.

Turned out to be a bug in egui (emilk/egui#2959). I haven't found any other issues.

@setzer22
Copy link
Owner

setzer22 commented May 8, 2023

Thanks a lot @hakolao and @kamirr for taking care of this! 😄 I have tested the changes and I can't spot any UX issues on my end either. I'll have a quick look at the code and then we can merge this 👍

Copy link
Owner

@setzer22 setzer22 left a comment

Choose a reason for hiding this comment

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

Everything looks good :) Changes are pretty straightforward so no objections on my end. And it looks like this closes #71!

@setzer22 setzer22 mentioned this pull request May 8, 2023
@setzer22 setzer22 merged commit b914f16 into setzer22:main May 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants