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

Fix ColorPicker deferred mode not working for sliders. #80916

Conversation

ajreckof
Copy link
Member

Fixes #80915

@ajreckof ajreckof requested a review from a team as a code owner August 23, 2023 06:26
@dalexeev dalexeev added this to the 4.2 milestone Aug 23, 2023
scene/gui/color_picker.h Outdated Show resolved Hide resolved
@ajreckof ajreckof force-pushed the fix-deferred-mode-not-working-for-color-picker-sliders branch from 2b14e2c to 08af262 Compare August 23, 2023 08:52
@KoBeWi
Copy link
Member

KoBeWi commented Aug 23, 2023

Clicking the slider does not emit changed signal in deferred mode. You need to drag it at least 1 px. SpinBox is also broken (note that you can drag SpinBox too).

@ajreckof ajreckof force-pushed the fix-deferred-mode-not-working-for-color-picker-sliders branch from 08af262 to 9816b31 Compare August 23, 2023 14:29
@KoBeWi
Copy link
Member

KoBeWi commented Aug 23, 2023

It's fine now, but there is a little inconsistency. Clicking the slider for the first time emits the signal immediately, while it doesn't happen when clicking the color box/circle. This is still an improvement, so I think that small inconsistency is ok (unless you can fix it easily?).
Dragging SpinBox still emits signal continuously. SpinBox doesn't expose any drag signals though, so it can be fixed later.

@ajreckof
Copy link
Member Author

It's fine now, but there is a little inconsistency. Clicking the slider for the first time emits the signal immediately, while it doesn't happen when clicking the color box/circle. This is still an improvement, so I think that small inconsistency is ok (unless you can fix it easily?).

I found it just after pushing I was looking at it. I'm not 100% sure why it is happening but should be pretty easy to fix

Dragging SpinBox still emits signal continuously. SpinBox doesn't expose any drag signals though, so it can be fixed later.

Yeah that was what I was going to say the class feels a bit lacking :(

@ajreckof ajreckof force-pushed the fix-deferred-mode-not-working-for-color-picker-sliders branch 2 times, most recently from 81ce565 to ed27c75 Compare August 23, 2023 17:18
@ajreckof
Copy link
Member Author

So I fixed the slider trigerring color_changed signal at the start of dragging, and at the same time it made me realise and fix that when clicking the slider it would trigger the signal twice.

@ajreckof ajreckof force-pushed the fix-deferred-mode-not-working-for-color-picker-sliders branch 2 times, most recently from cefb93d to b916c24 Compare August 23, 2023 17:38
@KoBeWi
Copy link
Member

KoBeWi commented Aug 23, 2023

SpinBoxes are broken in deferred mode again 🙃

@ajreckof ajreckof force-pushed the fix-deferred-mode-not-working-for-color-picker-sliders branch from b916c24 to db8fb5a Compare August 23, 2023 22:26
scene/gui/color_picker.h Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Works correctly now, left a few comments.

The Range changes make _drag_started() emitted before value changed. Probably it's fine though.

@ajreckof
Copy link
Member Author

I added both of your recommendation
I'm not 100% confident on the Range change but it is needed. I'll try to be as clear as possible on the reason that lead me to this modification.
So first since the SpinBox and the Slider share the same range whenever you modify one the other is updated but also both emit the value_changed signal. This means when you receive a value_changed signal from one you don't wether it is from one or the other.
What we want to achieve is that in deferred mode only at the drag end color_changed so the easiest idea is to just prevent the emission of the signal when the value_changed signal is received and emit the signal when the drag_ended signal is received. Unfortunately because of what I said earlier this isn't possible because to prevent the emission of the signal when the value_changed is received you can't differentiate between it being emitted from one or the other, therefore you will just never emit color_changed when changing value through the SpinBox.
So the other way is to detect when you are effectively doing a drag which means between the reception of drag_started and the reception of drag_ended. This works well but unfortunately this has a secondary problem which is the first value_changed signal is emitted before the drag_started signal which in trns mean you won't be able to know it is from the drag. So I reordered them but as to reduce the change I made sure that only their order were changed. Which means I had to update the value of the slider first without emitting the value_changed signal and then first emitting drag_started and then emit the value_changed signal.

If SpinBox ever gets drag_started/drag_ended signals just connecting them to the same function will fix the drag for SpinBox too.

@ajreckof ajreckof force-pushed the fix-deferred-mode-not-working-for-color-picker-sliders branch 3 times, most recently from f64fb26 to 636a40a Compare August 24, 2023 17:28
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

I see this adds more _no_signal helpers. We should aim to resolve the discussion on #76432 before adding more methods like this (though this one isn't exposed so it's not too critical).

scene/gui/slider.cpp Outdated Show resolved Hide resolved
scene/gui/slider.cpp Outdated Show resolved Hide resolved
@ajreckof ajreckof force-pushed the fix-deferred-mode-not-working-for-color-picker-sliders branch from 636a40a to 3e0eacb Compare September 19, 2023 08:47
scene/gui/range.h Outdated Show resolved Hide resolved
@ajreckof ajreckof force-pushed the fix-deferred-mode-not-working-for-color-picker-sliders branch from 3e0eacb to 3160add Compare October 13, 2023 12:57
@akien-mga akien-mga merged commit 74d7796 into godotengine:master Oct 13, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

Color picker sliders modification will call color changed signal even when deferred mode is active
4 participants