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

[Legacy color picker] Use zoomed preview at the mouse position instead of Label. #98411

Closed
wants to merge 1 commit into from

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Oct 22, 2024

Replaces the label with a 3x zoom preview that is following mouse cursor.

Screen.Recording.2024-10-22.at.11.15.10.mov

Can be combined with #88950 to do the same for a normal color picker.

@bruvzg bruvzg added this to the 4.x milestone Oct 22, 2024
@bruvzg bruvzg force-pushed the legacy_picker_zoom branch from 91b4f30 to a016477 Compare November 19, 2024 07:33
@bruvzg bruvzg marked this pull request as ready for review November 19, 2024 07:48
@bruvzg bruvzg requested a review from a team as a code owner November 19, 2024 07:48
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
@bruvzg bruvzg force-pushed the legacy_picker_zoom branch from a016477 to c5fb933 Compare November 25, 2024 13:54
@Calinou
Copy link
Member

Calinou commented Nov 26, 2024

I get a compilation error on MSVC 2022:

Compiling scene\gui\popup_menu.cpp ...
scene\gui\color_picker.cpp(1576): error C2027: use of undefined type 'Panel'
C:\Users\Hugo\Documents\Git\godotengine\godot\scene/gui/control.h(42): note: see declaration of 'Panel'
scene\gui\color_picker.cpp(1576): error C2672: '_post_initialize': no matching overloaded function found
C:\Users\Hugo\Documents\Git\godotengine\godot\core/os/memory.h(120): note: could be 'T *_post_initialize(T *)'
scene\gui\color_picker.cpp(1577): error C2027: use of undefined type 'Panel'
C:\Users\Hugo\Documents\Git\godotengine\godot\scene/gui/control.h(42): note: see declaration of 'Panel'
scene\gui\color_picker.cpp(1578): error C2027: use of undefined type 'Panel'
C:\Users\Hugo\Documents\Git\godotengine\godot\scene/gui/control.h(42): note: see declaration of 'Panel'
scene\gui\color_picker.cpp(1579): error C2664: 'void Node::add_child(Node *,bool,Node::InternalMode)': cannot convert argument 1 from 'Panel *' to 'Node *'
scene\gui\color_picker.cpp(1579): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or parenthesized function-style cast
C:\Users\Hugo\Documents\Git\godotengine\godot\scene/main/node.h(455): note: see declaration of 'Node::add_child'
scene\gui\color_picker.cpp(1579): note: while trying to match the argument list '(Panel *)'
scene\gui\color_picker.cpp(1581): error C2027: use of undefined type 'Panel'
C:\Users\Hugo\Documents\Git\godotengine\godot\scene/gui/control.h(42): note: see declaration of 'Panel'
scene\gui\color_picker.cpp(1581): error C2672: '_post_initialize': no matching overloaded function found
C:\Users\Hugo\Documents\Git\godotengine\godot\core/os/memory.h(120): note: could be 'T *_post_initialize(T *)'
scene\gui\color_picker.cpp(1582): error C2027: use of undefined type 'Panel'
C:\Users\Hugo\Documents\Git\godotengine\godot\scene/gui/control.h(42): note: see declaration of 'Panel'
scene\gui\color_picker.cpp(1583): error C2027: use of undefined type 'Panel'
C:\Users\Hugo\Documents\Git\godotengine\godot\scene/gui/control.h(42): note: see declaration of 'Panel'
scene\gui\color_picker.cpp(1584): error C2027: use of undefined type 'Panel'
C:\Users\Hugo\Documents\Git\godotengine\godot\scene/gui/control.h(42): note: see declaration of 'Panel'
scene\gui\color_picker.cpp(1585): error C2027: use of undefined type 'Panel'
C:\Users\Hugo\Documents\Git\godotengine\godot\scene/gui/control.h(42): note: see declaration of 'Panel'
scene\gui\color_picker.cpp(1591): error C2027: use of undefined type 'Panel'
C:\Users\Hugo\Documents\Git\godotengine\godot\scene/gui/control.h(42): note: see declaration of 'Panel'
scene\gui\color_picker.cpp(1594): error C2027: use of undefined type 'Panel'
C:\Users\Hugo\Documents\Git\godotengine\godot\scene/gui/control.h(42): note: see declaration of 'Panel'
scene\gui\color_picker.cpp(1597): error C2027: use of undefined type 'Panel'
C:\Users\Hugo\Documents\Git\godotengine\godot\scene/gui/control.h(42): note: see declaration of 'Panel'
scene\gui\color_picker.cpp(1666): error C2027: use of undefined type 'Panel'
C:\Users\Hugo\Documents\Git\godotengine\godot\scene/gui/control.h(42): note: see declaration of 'Panel'
scons: *** [scene\gui\color_picker.windows.editor.x86_64.obj] Error 2
scons: building terminated because of errors.

@akien-mga akien-mga requested a review from KoBeWi December 2, 2024 15:45
@bruvzg bruvzg force-pushed the legacy_picker_zoom branch from c5fb933 to 5386153 Compare December 3, 2024 06:41
@bruvzg
Copy link
Member Author

bruvzg commented Dec 3, 2024

I get a compilation error on MSVC 2022

Rebased and updated, this should be fixed now.

@Riteo
Copy link
Contributor

Riteo commented Dec 3, 2024

Hi, note that this causes a regression with the canvas_items scaling mode. See #97212.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 5, 2024

You can use minimum size instead of size, so ignore will work correctly.

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
@bruvzg bruvzg force-pushed the legacy_picker_zoom branch from 5386153 to 9ab78ec Compare December 5, 2024 13:21
@bruvzg
Copy link
Member Author

bruvzg commented Dec 5, 2024

Hi, note that this causes a regression with the canvas_items scaling mode.

Working in canvas_items scaling mode should be fixed.

Zoom preview is a bit blurry in a scaled mode.
Screenshot 2024-12-05 at 15 23 48

@Calinou
Copy link
Member

Calinou commented Dec 5, 2024

Zoom preview is a bit blurry in a scaled mode.

This may be due to #86563. I've been meaning to investigate that one for a while, likely using RenderDoc to make sure the glyph texture sizes match the size that should be used according to the oversampling factor and viewport resolution.

Comment on lines +1663 to +1664
Ref<Image> zoom_previw_img = img->get_region(Rect2i(ofs.x - 8, ofs.y - 8, 17, 17));
picker_texture_zoom->set_texture(ImageTexture::create_from_image(zoom_previw_img));
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that you are extracting an image only to make a texture. You could use AtlastTexture instead and set region.

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.

Looks like a nice improvement. The non-legacy picking could get the same treatment, but it's more tricky.

I added one more comment, but it's not super critical.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

@bruvzg
Copy link
Member Author

bruvzg commented Dec 6, 2024

The non-legacy picking could get the same treatment, but it's more tricky.

I have updated #88950 to do the same for native picker.

@Repiteo Repiteo modified the milestones: 4.x, 4.4 Dec 9, 2024
@bruvzg
Copy link
Member Author

bruvzg commented Dec 10, 2024

Superseded by #88950 (not includes the same for both native and legacy pickers).

@bruvzg bruvzg closed this Dec 10, 2024
@AThousandShips AThousandShips removed this from the 4.4 milestone Dec 10, 2024
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.

6 participants