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

Add outline to ruler tool #36530

Merged
merged 1 commit into from
Dec 15, 2020
Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Feb 25, 2020

Not sure about this tbh, but at least it solves an issue. It seriously triggered me.

Before:
image
After:
image

Probably resolves #33523 and resolves #42988

@Calinou
Copy link
Member

Calinou commented Feb 25, 2020

Try adding a 1-pixel offset on each side of the backdrop so the text doesn't "touch" any of its edges. This may look a bit better. Other than that, the feature looks good to me 🙂

@YeldhamDev
Copy link
Member

Wouldn't be better for the text to just have a stronger outline?

@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 25, 2020

Ok, increased the rect by 1 pixel
image
but I can't push changes right now for some reason.

@KoBeWi KoBeWi force-pushed the text_with_backdrop branch from c113e27 to 1e77186 Compare February 25, 2020 18:59
Copy link

@marcotmotta marcotmotta left a comment

Choose a reason for hiding this comment

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

I like the feature. Also good code formatting.

@YeldhamDev
Copy link
Member

Gonna sound like a broken record, but would be nice to have it be an outline instead. Would be subtler, and be consistent with #42995.

@groud
Copy link
Member

groud commented Nov 27, 2020

Ah damn, sorry I didn't see your comment. Indeed, a simple oultine might be better IMHO.
It seems quite easy to implement too, so guess it makes sense.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 27, 2020

ytoghVmGOp
Is this alright?

I copied the code from #42995.

@groud
Copy link
Member

groud commented Nov 27, 2020

Could you maybe try with a slightly larger outline? It seems that in some cases the outline ends up not rendered.

@Calinou
Copy link
Member

Calinou commented Nov 27, 2020

If you use an outline, consider using the editor's bold font so that the "white" part of the font is more visible. Right now, the text isn't easy to read.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 27, 2020

0kcu9k9Pdp
It already uses the bold font.

@groud
Copy link
Member

groud commented Nov 27, 2020

It's weird that the outline get such a a weird rendering. But anyway, I believe it's legible enough so that's fine to me.

@KoBeWi KoBeWi force-pushed the text_with_backdrop branch from 1e77186 to a9fd1f0 Compare November 27, 2020 16:24
@KoBeWi KoBeWi changed the title Add background rect to ruler tool Add outline to ruler tool Nov 27, 2020
@rsubtil
Copy link
Contributor

rsubtil commented Dec 13, 2020

I noticed this happening with the zoom label too some time ago. Apparently, the outline is "desync-ed" and is slightly smaller than the original text:
image
(top is the zoom label, the bottom is a label from a scene with the same font and outline settings)

I have no clue why this is happening, I inspected some variables in the debugger, and the outline's size matched the font's size.

However, with the new font rewrite, this was fixed, by changing to this:

zoom_reset->add_theme_constant_override("outline_size", 1);
zoom_reset->add_theme_color_override("font_outline_modulate", Color(0, 0, 0));

@KoBeWi KoBeWi force-pushed the text_with_backdrop branch from a9fd1f0 to 2254849 Compare December 14, 2020 22:27
@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 14, 2020

Rebased. The new font changes made the code even bigger :/ but at least it looks much better now
image

@KoBeWi KoBeWi force-pushed the text_with_backdrop branch from 2254849 to f2751f4 Compare December 14, 2020 22:29
@akien-mga akien-mga added this to the 4.0 milestone Dec 15, 2020
@akien-mga akien-mga merged commit f9d922b into godotengine:master Dec 15, 2020
@akien-mga
Copy link
Member

Thanks!

@GTcreyon
Copy link
Contributor

Any chance of this being present in the next version? Perhaps I'm missing something, but it's definitely not here right now.

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 18, 2021

This was never backported to 3.x, because it uses some functionality that doesn't exist there. It would need a separate PR.

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