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

Add layer transforms, interaction in layer #3906

Merged
merged 39 commits into from
Feb 17, 2024
Merged

Conversation

Tweoss
Copy link
Contributor

@Tweoss Tweoss commented Jan 27, 2024

⚠️ Removes Context::translate_layer, replacing it with a sticky set_transform_layer

Adds the capability to scale layers.
Allows interaction with scaled and transformed widgets inside transformed layers.

I've also added a demo of how to have zooming and panning in a window (see the video below).

This probably closes #1811. Having a panning and zooming container would just be creating a new
Area with a new id, and applying zooming and panning with ctx.transform_layer.

I've run the github workflow scripts in my repository, so hopefully the formatting and cargo cranky is satisfied.

I'm not sure if all call sites where transforms would be relevant have been handled. This might also be missing are transforming clipping rects, but I'm not sure where / how to accomplish that. In the demo, the clipping rect is transformed to match, which seems to work.

Screen.Recording.2024-01-28.at.18.29.39.mov

@Tweoss Tweoss marked this pull request as ready for review January 29, 2024 02:36
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Very cool stuff!

crates/egui/src/context.rs Outdated Show resolved Hide resolved
crates/egui/src/context.rs Outdated Show resolved Hide resolved
crates/egui/src/context.rs Outdated Show resolved Hide resolved
crates/egui/src/layers.rs Outdated Show resolved Hide resolved
crates/egui/src/layers.rs Outdated Show resolved Hide resolved
crates/egui_demo_lib/src/demo/pan_zoom.rs Outdated Show resolved Hide resolved
crates/egui_demo_lib/src/demo/pan_zoom.rs Outdated Show resolved Hide resolved
crates/egui_demo_lib/src/demo/pan_zoom.rs Outdated Show resolved Hide resolved
crates/epaint/src/mesh.rs Outdated Show resolved Hide resolved
crates/egui/src/context.rs Outdated Show resolved Hide resolved
@Tweoss
Copy link
Contributor Author

Tweoss commented Feb 1, 2024

@emilk Thank you for all the detailed comments and advice! I've taken some time and (hopefully) addressed all of them, could you take another look?

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Looking much better!

crates/egui/src/context.rs Outdated Show resolved Hide resolved
crates/egui/src/context.rs Outdated Show resolved Hide resolved
crates/egui/src/memory.rs Outdated Show resolved Hide resolved
crates/egui/src/memory.rs Outdated Show resolved Hide resolved
crates/epaint/src/shape.rs Outdated Show resolved Hide resolved
crates/emath/src/ts_transform.rs Show resolved Hide resolved
crates/emath/src/ts_transform.rs Outdated Show resolved Hide resolved
crates/emath/src/ts_transform.rs Outdated Show resolved Hide resolved
crates/emath/src/ts_transform.rs Outdated Show resolved Hide resolved
crates/emath/src/ts_transform.rs Show resolved Hide resolved
@Tweoss
Copy link
Contributor Author

Tweoss commented Feb 1, 2024

I added a few doctests, switched the transform order as you suggested, and completed the small refactoring tasks.

I think all the tests should be passing as well. Let me know if there's any other changes you'd like me to make. (I'm not sure if you meant for me to add doctests or a test module, I went with doctests for now)

@Tweoss Tweoss requested a review from emilk February 2, 2024 18:05
@Tweoss
Copy link
Contributor Author

Tweoss commented Feb 8, 2024

@emilk Could you take a look at the updates when you have a chance? I'd love to get this merged (and avoid merge conflicts 😛 )

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Just some docstrings to fix! Good job :)

crates/egui_demo_lib/src/demo/pan_zoom.rs Outdated Show resolved Hide resolved
crates/emath/src/ts_transform.rs Outdated Show resolved Hide resolved
crates/emath/src/ts_transform.rs Outdated Show resolved Hide resolved
crates/emath/src/ts_transform.rs Show resolved Hide resolved
crates/emath/src/ts_transform.rs Outdated Show resolved Hide resolved
crates/emath/src/ts_transform.rs Outdated Show resolved Hide resolved
crates/epaint/src/shape.rs Outdated Show resolved Hide resolved
@MeGaGiGaGon
Copy link

@Tweoss Thanks for the fix, I hadn't noticed that either. What I was referring to is how the Area dragging isn't synced, which sadly wasn't fixed, since it looks to access the InputState directly instead of through a Response.

if movable && move_response.dragged() {
    state.pivot_pos += ctx.input(|i| i.pointer.delta());
}
Video.mp4

Hopefully it is fixable inside of InputState/PointerState, since if not any custom widget that accesses the pointer position through ui/ctx .input would also be broken.

crates/egui/src/response.rs Outdated Show resolved Hide resolved
@emilk emilk added the egui label Feb 16, 2024
@emilk
Copy link
Owner

emilk commented Feb 16, 2024

The text clip bug is quite annoying.

It would also be good if the demo/example has a description in the UI (e.g. a label at the top, that isn't zoomed/panned)

@MeGaGiGaGon
Copy link

The two other biggest annoyances I've seen are that remain are the internal Areas not moving with the outer Window's movement, and the layering issues. I tried looking into both, but got stumped on changing an Area's pivot_pos since it is in State which is private. I also couldn't find any method of getting the ordering of the window containing the ui, though since Areas stores that in order: Vec<LayerId> then you'd just have to ensure the windows in the PanZoom are directly after the location of the containing window. This seems to more be a symptom of there not being a good interface for setting the ordering of areas/paintings, but that's probably something for a separate issue/pull.

Video.mp4

It would also be nice if there was an example of painting a shape onto the transforming layer, since painting onto the top level ui just makes it not zoom/pan, and having to make a static area to paint on/for every paint seems clunky.

@Tweoss
Copy link
Contributor Author

Tweoss commented Feb 17, 2024

@MeGaGiGaGon Is this the behaviour you're referring to where the inner windows should have their transformation match the outer demo window?

Screen.Recording.2024-02-16.at.15.58.24.mov

Unfortunately, I'm also not sure of a good solution to ensuring that the inner windows stay atop the demo window but not other demo windows.

@MeGaGiGaGon
Copy link

MeGaGiGaGon commented Feb 17, 2024

@Tweoss Yep, that's it :)
I think it would be possible, if not for two things:
Areas.order is private, otherwise you could edit it to make the order correct using ui.ctx().memory_mut
I have no clue how to get the LayerID of the window containing the PanZoom, especially with just access to the ui


Edit: On further though, it is possible to re-order Areas.order arbitrarily with the existing interface, though it is very hacky. Since Context::move_to_top exists, that is enough. I don't fully understand which way Areas.order has as more on top, but if this is backward, it can just be reversed pretty easily. It could be done by something like this

0. Store a list of all the `LayerID`s of the `Area`s inside the `ZoomPan`
1. Push all the `LayerID`s that are behind the `ZoomPan` to the front in order
2. Push the `LayerID` of the `ZoomPan`, then all of it's internal `Area`s to the front, in order
3. Push all the `LayerID`s that are in front of the `ZoomPan` to the front in order

This all means that, even though it is hacky, all that is needed is to get the LayerID of the containing window, and having the Areas be properly layered is possible in the existing interface.

@Tweoss
Copy link
Contributor Author

Tweoss commented Feb 17, 2024

Regarding the text disappearing, I've finally discovered that it is the initial text boxes' bounding box that goes beyond the clip rectangle, causing the whole text to disappear. I'll try to transform that bounding box as well.


Tada!

Screen.Recording.2024-02-16.at.22.12.25.mov

@MeGaGiGaGon
Copy link

Yay!


It's probably better to have the smiley be on it's own un-interactable area, since otherwise odd things happen with the widget it's painted on

image

It would also be nice to have an example painting that also moves with a draggable area


I did even more thinking, and sadly my idea wouldn't work. Getting the LayerId of the window isn't too bad if some spaghetti is allowed, here's my flavor:

pub struct PanZoom {
    transform: TSTransform,
    drag_value: f32,
    window_layer_id: Option<egui::LayerId>,
}
// ...
// inside impl super::Demo for PanZoom inside fn show
self.window_layer_id = window.show(ctx, |ui| self.ui(ui)).and_then(|x| Some(x.response.layer_id));
// ...
// inside fn ui
if self.window_layer_id.is_none() {
    return;
}

However the issue is still the Area ordering. As far as I can tell, the only tool that can inspect the Area.order is

ui.ctx().memory(|mem| mem.areas().top_layer_id(egui::Order::Middle))

This is sadly not powerful enough, since the only tool for manipulating the order is

 ui.ctx().move_to_top(layer_id)

Which also affects the top most Area. The only two things I can think of that would make it possible are having Areas.order be public, or having a Context::move_to_bottom method, which would still cause a lot of spaghetti in this code, but wouldn't expose Areas's internals.


Something else I just noticed is that things start glitching out if you zoom in far enough. I'd suspect this is due to floating point imprecision, so meh.

Video.mp4

I also also just noticed that while the translation is saved on close and open, the scale isn't

@Tweoss
Copy link
Contributor Author

Tweoss commented Feb 17, 2024

If people want to make their own window ordering, I think that could be separate and that the demo doesn't necessarily need to demonstrate that.

I should note that, though, some widgets may still need their mouse input coordinates to be reverse transformed, like in the panels and area widgets.

@MeGaGiGaGon
Copy link

MeGaGiGaGon commented Feb 17, 2024

What I'm mostly thinking about is if this were to be separated into it's own container/widget struct. I imagine most people making a PanZoom would just copy all the complex internal logic into their own struct (like I am) and have a few fns for their own specific use case. Granted, I have absolutely no clue how to make a good interface, and it looks like a lot of work that might be better suited for a later PR so that the actual TSTransform can make it into egui sooner rather than later.

Also on more thinking, one of these days there should also be a PR for making Areas.order more accessible, since that's the main issue that's being run into, and is probably outside the scope of this one. With what I've seen on how Memory added move_to_top, I might make a stab at it.

@emilk
Copy link
Owner

emilk commented Feb 17, 2024

This seems to be working quite well now.

The layering issue is difficult, and I think should be fixed in a separate PR.

Potential solutions include:

  • Add a good API for reordering layers, so we can tell the scroll-pan-layer to always be just on top of the demo window.
  • Add sub-layers, so each window can have multiple layers that are always connected to each other. This could also be the solution for Z-ordering of shapes #1516

In any case, I suggest we merge this now.

@emilk emilk merged commit 069d7a6 into emilk:master Feb 17, 2024
19 checks passed
@emilk
Copy link
Owner

emilk commented Feb 17, 2024

Oops - unfortunately this broke the drag-and-drop preview code. I'll see if I can fix it.

emilk added a commit that referenced this pull request Feb 17, 2024
@Tweoss Tweoss deleted the transform-layer branch February 17, 2024 17:22
@Tweoss
Copy link
Contributor Author

Tweoss commented Feb 17, 2024

Thank you @emilk for taking the time to review and add changes!

Also, thank you @MeGaGiGaGon for the suggestions and noticing those bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A panning / zooming container
3 participants