Fix preset cache usage in ColorPicker#104496
Conversation
FeniXb3
force-pushed
the
fix-color-palette-cache
branch
from
9c864b4 to
eb47eef
Compare
FeniXb3
force-pushed
the
fix-color-palette-cache
branch
from
eb47eef to
240861f
Compare
scene/gui/color_picker.cpp
Outdated
| for (int i = 1; i < preset_container->get_child_count(); i++) { | ||
| preset_container->get_child(i)->queue_free(); | ||
| } | ||
| if (presets.is_empty() || Engine::get_singleton()->is_editor_hint()) { |
There was a problem hiding this comment.
I think this points at the main problem here.
The reported issue was about standalone ColorPicker, while the preset update code assumes that the ColorPicker always comes from ColorPickerButton. I think we should always update the presets in the latter case. For that the ColorPicker needs to know whether it's standalone or from a button.
There was a problem hiding this comment.
Are you sure ColorPicker should behave differently depending on being standalone or from ColorPickerButon? I think it would be confusing, for me at least. I would expect them to behave in the same way. I am not sure if making it behave differently in game and in editor is a good idea for the same reason, but I think we might want to use them differently when letting players to use them.
Additionaly, with palettes we have some confusing cases now (since 4.4 till 4.5 dev 2) - if you have 2 ColorPickerButtons, load/save a palette in one and clear swatches in another one, the first one still has the name of a palette. If we simply add/remove a color in the second one, the first one's palette will not be marked as changed. See the videos below (project was running on 4.5 dev 2):
Nagrywanie.ekranu.2025-04-10.072822.mp4
Nagrywanie.ekranu.2025-04-10.073627.mp4
I think we should either make each in-game ColorPicker's presets independent regardless where it comes from (which is my current approach in this PR) or make all ColorPickers use the same data correctly and update presets and pallets in all, when any one of them is modified. Or, to let developers choose the behaviour they want in their game for each ColorPicker.
What do you think?
There was a problem hiding this comment.
The reason I think there should be difference is that you can't display 2 ColorPickers from buttons at once, while you can show multiple stand-alone ColorPickers. In the latter case, you may have multiple Swatches visible at once, so it does not make sense to update other ColorPickers (and it's not even feasible). In case of buttons, if you add a color to one picker, the other picker is not visible, so it's easy to sync them when the other button is clicked.
The palettes make it more complicated though. Ideally a Palette should sync across ColorPickers in every case, so if you have multiple ColorPickers visible at once (which again, is only possible with standalone ones), they should update their palette in real time. I think it might be possible by connecting to Palette's changed signal.
FeniXb3
force-pushed
the
fix-color-palette-cache
branch
from
240861f to
98a3ca1
Compare
FeniXb3
force-pushed
the
fix-color-palette-cache
branch
from
98a3ca1 to
97d82fd
Compare
akien-mga
force-pushed
the
fix-color-palette-cache
branch
from
97d82fd to
846ae11
Compare
|
Thanks! |
5 participants
Fixes #104223
It also fixes two additional bugs (click to expand)
ColorPickers. Video of incorrect behaviour from 4.4.stable:Nagrywanie.ekranu.2025-03-23.001706.mp4
ColorPickerin runtime. Video of incorrect behaviour from 4.4.stable:Nagrywanie.ekranu.2025-03-23.001306.mp4
Correct behaviour, fixed with this PR:
Nagrywanie.ekranu.2025-03-23.003851.mp4
Nagrywanie.ekranu.2025-03-23.004156.mp4