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

Editor: add gutter to preview and update inline colors in the script and shader editors #76171

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

poirierlouis
Copy link

@poirierlouis poirierlouis commented Apr 17, 2023

This is an implementation of this GIP #6653.

  • parses most Color constructors and static methods.
  • shows a ColorPicker when clicking on color preview's gutter. Set picker's position around color preview's gutter such as editor's content can still be visualized (may need Right-To-Left support in the future).
  • updates color in line with new picked color.
  • adds the editor setting text_editor/appearance/gutters/show_color_previews.
  • adds an SVG file, optimized with svgcleaner.
  • declares two editor icons.
  • updates documentation in doc/classes.
  • adds two unit tests (code coverage is limited).

Implementation is essentially private. Therefore only setter / getter to draw gutter are bound with ClassDB.

Not compatible with v3.x. I'd be willing to create another PR to support v3.x if this one were accepted.

Edit:

Production edit: Closes godotengine/godot-proposals#6653

@poirierlouis poirierlouis requested review from a team as code owners April 17, 2023 14:13
@poirierlouis poirierlouis force-pushed the color_preview_editors branch from 73fe19a to 334e5c2 Compare April 17, 2023 14:34
@poirierlouis
Copy link
Author

Fixed clang-format check failing, add missing new line to SVG file.

@poirierlouis
Copy link
Author

Fixed clang-format check failing, remove reference syntax [] off a theme_item tag.

@poirierlouis poirierlouis force-pushed the color_preview_editors branch from 952d027 to 6c5d136 Compare April 17, 2023 16:47
Copy link
Contributor

@RedMser RedMser left a comment

Choose a reason for hiding this comment

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

This is a great feature, looking forward to it! :D

Though the color constructor parsing/rewriting seems a bit hacky to me:

  • it assumes CodeEdit is only ever used for GDScript (it's not). I assume you want this feature to be supported by anyone using this UI element, not just the editor. Otherwise, look at modifying ScriptTextEditor.
  • any changes to Color constructors / static methods would need to be reflected here as well. This increases the cost of maintenance.

Some ideas I've come up with to solve these issues, but feel free to think of your own:

Could the color parsing maybe be changed to work similarly to the "Evaluate Selection" feature, by parsing the Color\(.+?\) expression, and using the resulting variant as input for the color picker? I don't actually know if the Expression class is capable of this, but I'd assume it would work, since it's a built-in variant type - needs testing.
Possibly allow overriding this logic for different languages, by adding a virtual method, something like func _parse_color_editor_value(line: String) -> Color | null.

And as for storing the result, it might be enough to have a virtual method like func _save_color_editor_value(color: Color) -> String which by default would be implemented using a single constructor of Color (for example, have it always use the hexadecimal syntax).

Or if you want to go crazy, couple this with a "preferred color format" editor setting where you can choose RGBA, leave out the alpha component if it's 100% opaque, etc.
But I don't see a need to keep the color syntax the same from what it initially was - VSCode for example does not do it like this.

scene/gui/code_edit.cpp Outdated Show resolved Hide resolved
scene/gui/code_edit.cpp Outdated Show resolved Hide resolved
@MewPurPur
Copy link
Contributor

MewPurPur commented Apr 17, 2023

Could you update the PR description with photos or videos of using it so we can take a quick cursory look?

I have some concerns regarding the implementation, mainly that having a whole gutter for this means a significant loss of horizontal space in the script editor. The info editor could be used for this, but then I feel like we'd also need some infrastructure around deciding what to show on that gutter and what to hide...

@poirierlouis
Copy link
Author

Could you update the PR description with photos or videos of using it so we can take a quick cursory look?

Here three screenshots with different positioning of the picker:

@Paulb23
Copy link
Member

Paulb23 commented Apr 17, 2023

Not a fan of adding it to the gutter, I was thinking we'd add support for a "drawing API" of some form to TextEdit first. Part of that would allowing adding inline icons taking advantage of the text server. We can use that API to add this functionally.

@YuriSizov
Copy link
Contributor

YuriSizov commented Apr 17, 2023

Fixed clang-format check failing, remove reference syntax [] off a theme_item tag.

You used formatting incorrectly. Only classes can be linked to with just [foo]. Members of classes need to use special tags, like [member foo], [method foo], [constant foo], or, in this case, [theme_item foo].

@poirierlouis
Copy link
Author

@Paulb23

Not a fan of adding it to the gutter, I was thinking we'd add support for a "drawing API" of some form to TextEdit first. Part of that would allowing adding inline icons taking advantage of the text server. We can use that API to add this functionally.

I did mention such possibility in the GIP, which brings another question. However there is no such "drawing API" to my knowledge. It would need another GIP / PR to discuss about the use-cases of such feature (at least this one but I can think of another). I think more work and experience with the engine would be required as it gets closer to the engine's core.


@RedMser

it assumes CodeEdit is only ever used for GDScript (it's not). I assume you want this feature to be supported by anyone using this UI element, not just the editor. Otherwise, look at modifying ScriptTextEditor.

My first intent was to just target the editor. But as I was searching, it was not clear to me what was what in the engine. Thank you for explaining this, I'll refactor and move rules to ScriptTextEditor.

any changes to Color constructors / static methods would need to be reflected here as well. This increases the cost of maintenance.

In deed, I though about this and the maintenance cost it induces. I think such changes is unlikely as it would also require Godot's projects to be refactored to support a new syntax (big downside). Nonetheless, I didn't know the engine enough to provide a better approach.

Could the color parsing maybe be changed to work similarly to the "Evaluate Selection" feature, by parsing the Color(.+?) expression, and using the resulting variant as input for the color picker? I don't actually know if the Expression class is capable of this, but I'd assume it would work, since it's a built-in variant type - needs testing.

Oh, great! I'll check this and test it, thank you for the feature's name.

Possibly allow overriding this logic for different languages, by adding a virtual method, something like func _parse_color_editor_value(line: String) -> Color | null.

I guess this would also apply while using ScriptTextEditor?

But I don't see a need to keep the color syntax the same from what it initially was - VSCode for example does not do it like this.

I beg to differ on this, as I proposed in GIP. It's a bit like clang-format with its configuration file. It forces the project to follow a set of formatting rules. But it can be a different set of rules on another project... What if Godot's user want to always write colors in a specific format? Much like a clang-format rule. Forcing a syntax would break such rule.

Or if you want to go crazy, couple this with a "preferred color format" editor setting where you can choose RGBA, leave out the alpha component if it's 100% opaque, etc.

Thinking about it, I like your idea, it should be a good compromise. It would enforce the proposed rule above for all users of a Godot project. I'll investigate this as I work on ScriptTextEditor.

Many thanks!

@poirierlouis poirierlouis force-pushed the color_preview_editors branch from 6c5d136 to 96d8730 Compare April 18, 2023 15:01
@poirierlouis
Copy link
Author

Improved documentation: format with special tag [theme_item tag].
Fixed read-only issue: prevent inline color update when text is not editable.

@poirierlouis
Copy link
Author

@RedMser , I tried with the Expression class. It doesn't work with static methods (not implemented in Expression).
Would it be ok to implement a static method to parse color formats somewhere "globally", maybe on the Color class?
Or should I start a new GIP / PR to add static method support on the Expression class itself (if it is even possible)?

@RedMser
Copy link
Contributor

RedMser commented Apr 19, 2023

@poirierlouis

It doesn't work with static methods

That's a shame.
I'm not sure what the next best approach would be that could avoid this additional code. I don't think there is an easy API to execute a GDScript snippet and get its result.

Would it be ok to implement a static method to parse color formats
Or should I start a new GIP / PR to add static method support on the Expression class itself

I'm not in a position to be able to decide or judge these things. But my recommendation would be, if you want this merged, to start out simple and have the chance to extend it later.

So maybe parsing just Color constructors via Expression is already enough for the MVP, and finding a nice solution to parse the static methods and named color properties could follow later. Same goes for the saving logic, which could start out by just using what the "Evaluate Selection" function uses internally to convert the result back into a String.

Maybe other more experienced contributors could chime in here and provide their thoughts.

@poirierlouis poirierlouis force-pushed the color_preview_editors branch from 96d8730 to ccacd14 Compare April 23, 2023 12:51
@poirierlouis
Copy link
Author

poirierlouis commented Apr 23, 2023

  • refactor: move logic to ScriptTextEditor only for now.
  • refactor: removed unit tests. There is no tests/editor/.
  • docs: only add description for the new "Show Color Preview Gutter" property.
  • feat: parse Color declaration using Expression, more limited than the "Pick Color" feature (see below).
  • feat: format to Color(r, g, b, a). Now "Color Preview" and "Pick Color" use the same formatting function.
  • feat: add function to compute the color panel's position with responsiveness.
  • feat: use metadata from the "Gutter API" as a cache to improve performance.
  • feat: only run this feature after the script is validated (like the "connection gutter").

Expression is limited for now. It does not support named color declarations. Which is a downside regarding the "Pick Color" feature. It parses float constructors and named colors manually within ScriptTextEditor.

Moreover, I figured there is an "issue" when parsing other syntaxes like @export var color := Color.hex(0xFFA800). It fallback to a black color in the "Inspector".
The idea of proposing a single endpoint to parse / format GDScript colors could be used in at least three use-cases:

  • @export a Color
  • Pick Color
  • Color Preview (this)

@poirierlouis poirierlouis force-pushed the color_preview_editors branch from ccacd14 to c64f72e Compare April 23, 2023 18:01
@poirierlouis
Copy link
Author

  • feat: add logic to parse named colors.
  • refactor: replace parser logic in "Pick Color" with a call to _parse_color_value method.
  • fix: update metadata of a line's color preview gutter after color changed with "Color Picker".

Video of this current commit here, steps:

  1. Use "Pick Color".
  2. Use "Color Preview".
  3. Cancel step 2 and step 1 with CTRL + Z, check color preview is refreshed (delay is due to script validation process).
  4. Show responsiveness of Color Picker around color preview gutter.

@poirierlouis poirierlouis marked this pull request as draft April 24, 2023 15:26
@poirierlouis poirierlouis force-pushed the color_preview_editors branch from c64f72e to 1b8a46f Compare April 25, 2023 11:29
@poirierlouis
Copy link
Author

  • feat: add color preview in ShaderTextEditor, it is a duplicate of the logic from ScriptTextEditor but it only parses the vec4() syntax with source_color hint.

@RedMser
I'm wondering if it is ok for now to have almost identical code between ShaderTextEditor and ScriptTextEditor? Or if it must be merged somehow, maybe within CodeEdit?

@KoBeWi
Copy link
Member

KoBeWi commented Apr 25, 2023

'm wondering if it is ok for now to have almost identical code between ShaderTextEditor and ScriptTextEditor?

See #28607 Code editors already duplicate lots of code, so it's fine until they are refactored.

@poirierlouis poirierlouis force-pushed the color_preview_editors branch from 1b8a46f to b8c320a Compare April 25, 2023 13:26
@poirierlouis
Copy link
Author

  • build: fix variable name in method shadowing a class member variable.

@poirierlouis poirierlouis force-pushed the color_preview_editors branch from b8c320a to bf934b1 Compare April 26, 2023 08:08
@poirierlouis poirierlouis marked this pull request as ready for review April 26, 2023 08:08
@poirierlouis poirierlouis requested a review from a team as a code owner April 26, 2023 08:08
@poirierlouis poirierlouis force-pushed the color_preview_editors branch 2 times, most recently from 8d91ebd to 2596e7c Compare April 27, 2023 17:32
@poirierlouis poirierlouis force-pushed the color_preview_editors branch from 2596e7c to 3b87a8a Compare May 30, 2023 10:26
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.

Preview and update inline colors with a ColorPicker in the script and shader editors
6 participants