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

Highlight scripts used by current scene #97041

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

aXu-AP
Copy link
Contributor

@aXu-AP aXu-AP commented Sep 15, 2024

@Mickeon
Copy link
Contributor

Mickeon commented Sep 20, 2024

I see overwhelming positivity behind this PR (not the proposal though). Perhaps it's worth continuing.

Although, we're still struggling to find a good, non-hardcoded theme colour for it. Current PR uses transparency for a dimmer look, but I don't know if that'll play nice on light themes.
Perhaps it could be the theme's highlight colour, but heavily mixed with the theme's background colour used by the list.

@aXu-AP aXu-AP force-pushed the script-highlight-scene branch from a7719ea to 87334f0 Compare September 20, 2024 16:56
@aXu-AP
Copy link
Contributor Author

aXu-AP commented Sep 20, 2024

Transparency isn't only for dimmer look, it also is needed for regular highlighting to work (it seems that item background is drawn on top of highlight box).
Here's highlight color with transparency down to 20%, various themes:
kuva
ai_component is the script selected, mouse is hovering over player. enemy and health_component are in the current scene.
Looks not bad in the default theme. In light theme, the color is quite close to mouse hover color. In black theme highlight is quite dim.
I think it's acceptable? I'm not too worried about being close to hover color. In motion it looks better.

@aXu-AP aXu-AP marked this pull request as ready for review September 20, 2024 16:58
@aXu-AP aXu-AP requested a review from a team as a code owner September 20, 2024 16:58
@Calinou
Copy link
Member

Calinou commented Sep 20, 2024

Current PR uses transparency for a dimmer look, but I don't know if that'll play nice on light themes.

You can use a transparent gray (Color(0.5, 0.5, 0.5, 0.1)) which will work well on any background color. It's fine to hardcode this one as it generally does not need to be ever changed, and is often specific to certain use cases.

@Mickeon
Copy link
Contributor

Mickeon commented Sep 20, 2024

I like the chosen look in the above screenshots, actually. I am concerned that the background on the fully black theme being completely invisible, though.

Having it be subtle but noticeable is nice. As you said, it looks better in motion. It pops out when changing scenes around but it's not distracting.

A friend has also expressed concern since this is restoring a feature users have gotten used to without. I believe most would see this as an upgrade, but it may warrant an Editor Setting, we'll see.

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Sep 20, 2024

Here's with transparent gray as suggested by @Calinou:
kuva
I think it's tad more readable. Of course it wouldn't work if you have background color of 50% gray, but that's quite rare, and none of default themes do this. Ableton Live is only program that comes to mind that has such color palette.
Editor setting sounds reasonable, as we have it with script temperature also. Some do like more vanilla list.

@aXu-AP aXu-AP force-pushed the script-highlight-scene branch from 87334f0 to a07131b Compare September 20, 2024 19:11
@aXu-AP aXu-AP requested a review from a team as a code owner September 20, 2024 19:11
@aXu-AP
Copy link
Contributor Author

aXu-AP commented Sep 20, 2024

Changed the color to Color(.5, .5, .5, .1) and added editor setting text_editor/script_list/highlight_scene_scripts now.

@aXu-AP aXu-AP force-pushed the script-highlight-scene branch from a07131b to ac726f7 Compare September 21, 2024 11:24
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

I'd say yeah as is. It's either this, or the feature is stripped out of the codebase because it currently is not working.

I have a gut feeling that the highlight will miss some scripts in the current scene, but we'll see. There's a huge desire to rework the script list someday, but that day is not as of writing this.

@akien-mga akien-mga requested a review from Calinou September 23, 2024 14:08
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. Built-in scripts work, as well as scripts from instanced subscenes (they will be highlighted as being part of the current scene, which is technically valid).

The highlighting is a bit too subtle for me to easily notice it though. I suggest increasing the opacity to 0.125.

@Mickeon Mickeon modified the milestones: 4.x, 4.4 Nov 4, 2024
@aXu-AP aXu-AP force-pushed the script-highlight-scene branch from ac726f7 to f168d0c Compare November 5, 2024 15:10
@aXu-AP aXu-AP requested a review from a team as a code owner November 5, 2024 15:10
@aXu-AP
Copy link
Contributor Author

aXu-AP commented Nov 5, 2024

Changed it to 0.125, I think it's about right. Comparison with varying degrees of opacity:
kuva

@Mickeon Mickeon requested a review from Calinou November 5, 2024 17:25
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.

Looks great!

@Repiteo Repiteo merged commit 04f3389 into godotengine:master Nov 12, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 12, 2024

Thanks!

@aXu-AP aXu-AP deleted the script-highlight-scene branch November 12, 2024 15:41
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.

Highlight scripts used by current scene
5 participants