Skip to content

Improve area positioning behaviour, and add new options for the auto positioning of moveable areas. #6754

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mkalte666
Copy link
Contributor

@mkalte666 mkalte666 commented Apr 15, 2025

When i did the quick implementation for movable modals for #6749 @lucasmerlin pointed out that that just forcibly recentering a modal if it is movable is a bit odd.

It would be nicer if this was an inherent property of Areas.

This is a surprisingly small change set, considering the wide impact it has, so i went with a pr directly, instead of an issue.

Investigating this, i figured that some area is quite Stateful in its positioning.

Instead of keeping default_pos, new_pos, anchor as seperate entities, i have implemented its internal position using a new enum, AreaPosition, that now encodes the different types of position an area can have.

  • Is it at a fixed position?
  • Is it anchored somewhere?
  • Is it moveable?
    • If it is indeed movable, what is its initial position?

I have also quickly thrown together a demo, that i will expand on with test cases.

The way it is done now is that all the old builder methods still exist and behave the same, though the colliding states (setting area movable while an anchor is set, for example) now overwrite each other. In practice this should not change anything, i think?

Things that are still to-do before this can be merged in the first place:

  • Anchored windows are currently not resizable, though they should be
  • Investigate how pivot affected anchors before, and mirror this behavior correctly
  • Investigate how pivot affected auto position before, and mirror that behavior correctly.
  • Clean up the demo, and the comments of the builder methods
  • Area::get_pivot previously only reported TOP_LEFT; or if set, the anchor. Was that correct? - im now reporting the pivot instead. Needs trying out.
  • Investigate the interactions between setting movability, and the intractable flag, and figure out what best reflects current behavior. There is likely only one correct solution.

@mkalte666 mkalte666 changed the title Area positioning Improve area positioning behaviour, and add new options for the auto positioning of moveable areas. Apr 15, 2025
Copy link

Preview available at https://egui-pr-preview.github.io/pr/6754-areapositioning
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

}
}

/// A moveable position that starts centered
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// A moveable position that starts centered
/// A moveable position that starts with a automatic position

Comment on lines +307 to 311
if !movable && self.is_movable() {
self.position = AreaPosition::anchored(Align2::CENTER_CENTER, [0.0, 0.0]);
}

self
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about the interactable flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... Oops , forgot that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logic wise, The "movable" window can still be positioned via override_pos (or current_pos). So i think a note saying "you can set this to movable, and disable interactions, so the window will never actually be movable in the end.

I don't know when you'd want this, though.

I'll think about this more.

Comment on lines +443 to +445
AreaPosition::Fixed(pos) => *pos = current_pos.into(),
AreaPosition::Anchored { .. } => {}
AreaPosition::Moveable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should add a debug_assert or at least a log warning when with the current position type something isn't supported?

Centered,

/// Put the area at a position. This will take [`Area::pivot`] into consideration.
Position(Pos2),
Copy link
Owner

Choose a reason for hiding this comment

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

Would it make sense to move pivot into here?

Copy link
Contributor Author

@mkalte666 mkalte666 Apr 22, 2025

Choose a reason for hiding this comment

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

That would make pivot only affect the initial position, instead of any current position. (i.g what is set via Area::current_position for overriding). That said, it might make sense to make it a property only of movable windows and fixed positioned windows - as far as i can see it anchoring always anchors corners to coners / center to center + offset. I'll try to incorporate this and the above points once i have time, though it might be a week or three until i find some again. sorry x.x

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

Successfully merging this pull request may close these issues.

3 participants