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

Pivot alignment #366

Closed
wants to merge 5 commits into from
Closed

Conversation

pgilfernandez
Copy link

@pgilfernandez pgilfernandez commented Dec 10, 2024

Hi,

I've been working on this new feature the last two weeks. It's not finished yet but it is full feature ready so I thought it was the moment to show it.

Till now, the only graphical option to align the object pivot was by opening the contextual menu and selecting "Center pivot":

screenshot

I extended the geometry alignment widget to allow pivot alignment. In the way I restructured the dropdowns and renamed "Pivot" to "Geometry by Pivot" as it really aligns the geometry using the pivot as the center point.

Now "Pivot" option introduces real pivot alignment with options to align it to Scene, Last selected, Last selected pivot and Bounding Box (most common use):

menus

Please, have a look at the video showing them all:

pivot_alignment1.mp4

I would say the feature state is the following:

  • align pivot relative to his bounding box
  • align pivot relative to scene
  • align pivot relative to last selected
  • align pivot relative to last selected pivot
  • cleaning up not used functions and/or simplify it all
  • make them undoable/redoable
  • fix known issue: text object doesn't work properly when "align pivot relative to last selected pivot" (It kinda but not as expected)
  • fix known issue: null object make Friction to crash when aligning pivot to any relative to option...

Help is appreciated as the remaining tasks could make me fall into a maze and loose a lot of time for probably simple fixes that I don't reach to find.... BTW, I'll continue trying, hehehe.

Cheers

Sorry, something went wrong.

@pgilfernandez pgilfernandez marked this pull request as draft December 10, 2024 16:29
@rodlie
Copy link
Member

rodlie commented Dec 10, 2024

Cool, will take a look later today or in a couple of days (a bit busy this week).

@rodlie
Copy link
Member

rodlie commented Dec 10, 2024

Note we are in a feature freeze, but depending on how intrusive this change will end up I may allow it in v1.0.

@pgilfernandez
Copy link
Author

Note we are in a feature freeze, but depending on how intrusive this change will end up I may allow it in v1.0.

Yes, I know... anyway, in case you decide to postpone it there is a chance to just add support for "align pivot relative to bounding box", that was the original feature I was going to add but I found myself going deeper and deeper creating more features...

@rodlie
Copy link
Member

rodlie commented Dec 10, 2024

As long as it does not touch anything related to load/save (project format) or introduce bugs/crashes this might end up in v1.0. I don't want to release another beta so it has to be RC-ready when/if merged into main.

Will have to look at the changes and what is marked as not done and how to fix that.

Copy link
Member

@rodlie rodlie left a comment

Choose a reason for hiding this comment

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

Did a quick test and it seems to work, will need to test more.

Also did a quick code review, will look more into it tomorrow. Will also help with code changes if needed.

connect(mAlignPivot, QOverload<int>::of(&QComboBox::currentIndexChanged), this, [this](int index) {
static bool isItem2Removed = true;

if (index == 2) {
Copy link
Member

Choose a reason for hiding this comment

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

hmm, "magic" numbers :)

Should probably add some defines instead of numbers, easier to figure out what's what.

I'm also not a big fan of insertItem, I would prefer addItem.

Regarding removeItem I think we need to get the index of what we are removing instead of using a fixed index.

Do we even need to remove anything? can't we hide/disable instead?

I can help figure out some logic to this.

Copy link
Author

Choose a reason for hiding this comment

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

I thought you preferred indexes, no problem, I can change it.

Do we even need to remove anything? can't we hide/disable instead?

Yes, we can hide but I'm not fan of leaving visible (even disabled) items that are never gonna be usable for an specific option. But it's just a personal UX decision, it can be done as you prefer

Copy link
Member

Choose a reason for hiding this comment

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

Wait with this, let figure out how to manage the UI/UX first.

@@ -121,6 +153,29 @@ AlignWidget::AlignWidget(QWidget* const parent)
});
buttonsLay->addWidget(bottomButton);

connect(mRelativeTo, QOverload<int>::of(&QComboBox::currentIndexChanged), this, [this, leftButton, rightButton, topButton, bottomButton]() {
if (mRelativeTo->currentIndex() == 2) {
Copy link
Member

Choose a reason for hiding this comment

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

More "magic" numbers :)

Copy link
Author

Choose a reason for hiding this comment

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

hehe, yes...

Copy link
Member

Choose a reason for hiding this comment

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

"Last Selected Pivot" does not support all options?

Also, "Last Selected Pivot" does nothing(?).

Copy link
Author

@pgilfernandez pgilfernandez Dec 27, 2024

Choose a reason for hiding this comment

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

Si you mean in this piece of code or the feature itself?

The feature works and moves all pivots to match vertical/horizontally the last selected object pivot

Copy link
Member

Choose a reason for hiding this comment

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

The feature works

Yes it does, my bad 😄

QPointF center = getRelCenterPosition();

if (relativeTo == AlignRelativeTo::scene) {
if (align & Qt::AlignVCenter) {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably use a switch. Not important, just wanted to note :)

Copy link
Author

Choose a reason for hiding this comment

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

OK

@@ -50,6 +50,7 @@ class UI_EXPORT AlignWidget : public QWidget

QComboBox *mAlignPivot;
QComboBox *mRelativeTo;
QComboBox *mRelativeToPivot;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

OK, I will

Copy link
Member

Choose a reason for hiding this comment

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

See my last comment, let's figure out what to do with the UI first.

Copy link
Member

Choose a reason for hiding this comment

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

btw, why does mRelativeToPivot exists?

Copy link
Author

Choose a reason for hiding this comment

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

Oh, I remember it now, it could be removed.
In my first development cycle I only implemented "relative to bounding box" and "relative canvas" and the technique I used was to hide the normal relativeTo comboBox and show the new one when "align to pivot" was selected in the other comboBox but now all options are used (Last selected ones) so this one could be removed... my fault not to realize about it before and clean it

Copy link
Author

Choose a reason for hiding this comment

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

Do you want me to do it?

@rodlie rodlie added this to the 1.0.0 milestone Dec 25, 2024
@rodlie rodlie added the enhancement New feature or request label Dec 25, 2024
rodlie added a commit that referenced this pull request Dec 25, 2024
@rodlie
Copy link
Member

rodlie commented Dec 25, 2024

Moved development to pivot-aligment2, added 215f106

I will go through the alignwidget later today.

@rodlie
Copy link
Member

rodlie commented Dec 25, 2024

alignwidget

I don't like the changes to the widget, vertical space is (always) limited.

@pgilfernandez
Copy link
Author

Hi!
I was totally out for Christmas holidays, I hope to be a little bit more connected in the following days.
I'm answering your comments and move any new PR to the new branch you made

@rodlie
Copy link
Member

rodlie commented Dec 27, 2024

No rush :)

Note that the new branch will probably not build on macOS, as I included a bad version of skia in main, will fix in a couple of hours.

@pgilfernandez
Copy link
Author

alignwidget

I don't like the changes to the widget, vertical space is (always) limited.

Yes, I know, I did it this way for two reasons:

  • I'm not fan of having options inside a combobox that repeats always the same "Align Geometry", "Align Geometry by Pivot" and "Align Pivot"
  • because "Align Geometry by Pivot" text gets too long and doesn't work very well with a narrow panel

So that I sacrificed a row to help thing to be clearer but if you don't like it I understand it. Do you have any idea to name options so that they are not that long and keep good UX?

@rodlie
Copy link
Member

rodlie commented Dec 27, 2024

Yes, I know, I did it this way for two reasons ...

Move the align stuff to it's own toolbar?

Let me test out some things before you do anything to the alignwidget.

@pgilfernandez
Copy link
Author

Don't worry, I'll leave it this way

@rodlie
Copy link
Member

rodlie commented Dec 27, 2024

UI suggestion:

friction-align-ui-2024-12-27_15.08.22.mp4

@pgilfernandez
Copy link
Author

It's fine to me, I like it.

I prefer removing the icons for "relative to selected pivot" as it makes it super clear but its fine disabling them as well (it's what I did on my first test)

rodlie added a commit that referenced this pull request Dec 27, 2024
@pgilfernandez
Copy link
Author

While doing more tests I found a bug in "align pivot" "to scene" when scale is not equal to 1... I'm gonna have a look at it.

Anything else you want me to have a look at?

@rodlie
Copy link
Member

rodlie commented Dec 27, 2024

While doing more tests I found a bug in "align pivot" "to scene" when scale is not equal to 1... I'm gonna have a look at it.

Ok.

Anything else you want me to have a look at?

I think we are good on the UI.

rodlie added a commit that referenced this pull request Dec 27, 2024
@rodlie
Copy link
Member

rodlie commented Dec 28, 2024

Since your initial commits are messy and I did several commits in a different branch I suggest this "final" commit: 94e1652

@rodlie rodlie removed this from the 1.0.0 milestone Dec 30, 2024
@rodlie rodlie closed this in #404 Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants