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

[WIP] dynamic DPI scaling #28771

Closed
wants to merge 10 commits into from
Closed

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented May 8, 2019

There are details about the implementation in this markdown ThemeDynamicDpi.md (Haven'tdone any 'read and correct' yet so it might be a mess... I still invite you to fight through it ;) )

And there is a tweet with a demo https://twitter.com/Toger50/status/1109870886671339521

@toger5 toger5 requested review from reduz and a team as code owners May 8, 2019 19:29
@swarnimarun
Copy link
Contributor

Should be what the proposal godotengine/godot-proposals#6 needs.

@fire
Copy link
Member

fire commented May 9, 2019

Tried a draft for visual script zooming.

https://github.com/fire/godot/tree/vs_zoom

Edited:

Updated for master.

@fire
Copy link
Member

fire commented May 9, 2019

@toger5
Copy link
Contributor Author

toger5 commented May 9, 2019

Nice ;)

@toger5
Copy link
Contributor Author

toger5 commented May 9, 2019

It might even be possible, to create some kind of container control that allows scaled displaying of anything without blurring fonts and styles. So u Don't necessarily need to use the graph control to make a open and zoom-able canvas

@fire
Copy link
Member

fire commented May 9, 2019

How would that work?

It seems like it should be a property of control.

@fire
Copy link
Member

fire commented May 10, 2019

DPI themes for Godot Engine

We've decided to generate zoom themes and stored editor wide.

The current prototype is to use the DPI scaling to do general scaling of the Theme and then for textures and style boxes use the scaled wrappers. I believe all the icons are svg so they should scale.

We've already discussed that instead of dpi scaling we want DPI themes. The wrappers for textures and styleboxes shouldn't exist. The way to reset the dpi is to go to another theme. However, I don't know how to solve the wrapper for textures and style box problem.

In this particular case for graph node, it won't work. Reduz suggests to modify GraphNode so instead of just zooming as it does now, you have to pass the Theme for each zoom level. If no zoom levels are passed, the zoom icons are disabled

GraphNode.add_zoom_level(0.5,theme_half_dpi)

It makes sense from the API point of view to have an array of zoom levels that map to dpi themes. How do you do the texture and style box scaling?

Reduz mentions that the editor has a function to create themes on any resolution already and these zoom levels could be already pre-created in editor_theme source.

Pass EDSCALE as an argument to the create_editor_theme function.

Ref<Theme> create_editor_theme(const Ref<Theme> p_theme) {

Generate the zoom levels when the editor starts (or changes Theme).

One doesn't need that many zoom levels, Reduz thinks something like 1.5, 1.0 (use existing), 0.75, 0.5 and 0.25 may be enough. Add these zoom levels to the GraphNode explicitly.

Use GraphNode.add_zoom_level(scale, theme) and make sure GraphNode removes zoom icons when no zoom levels are passed.

Make zooming editor wide. There is a visual shader editor, animation editors and others. That would benefit from the zoom levels too.

@ghost
Copy link

ghost commented May 10, 2019

Any idea what Blender is doing to make their node zooming so sharp and seamless? I understand using unique theme zooms, but it is far more limiting than Blender. I love the granularity you get in Blender.

@Calinou
Copy link
Member

Calinou commented May 10, 2019

@turtleTooth I believe Blender doesn't fiddle with themes when you change the scale, it just renders everything at a larger scale 🙂 And since GUI controls are vector-based (like in Godot), they remain crisp.

I think this is the way to go, but it'd require a lot of refactoring (in addition to removing the EDSCALE constant in profit of DPI-independent pixels, similar to dp units on Android). Ultimately, theme designers and implementers shouldn't have to think about scaling – the engine should do it for them.

@swarnimarun
Copy link
Contributor

@turtleTooth Blender's and Godot's Theme system are a bit different. They should be using the same way of display scaling but it's more dynamic in there case.
Very likely just use a direct factor to scale.

But in Godot it's baked in(we use macro), I have a test branch where I tried to remove EDSCALE as much as I can but seems like it's going to take quite a bit of refactoring.

@swarnimarun
Copy link
Contributor

swarnimarun commented May 10, 2019

@Calinou What's DPI Independent Pixels? Did you mean Device Independent Pixels?

Nvm got it. Density Independent Pixels makes sense... :)

@toger5
Copy link
Contributor Author

toger5 commented May 10, 2019

@fire @reduz I still dont see any advantages in multiple themes.

  • Worse performance. (this pr also increases drawing performance)
  • Takes more storage to set up (multiple versions of the theme with the same information)
  • Since stylebox flat themes are calculate theire drawing anyways they almost ask for dpi_scale beeing a property.
  • you can never have smooth transitions. + transitions to another dpi lvl are much slower.
  • It makes it possible to solve the graph node drawing properly. With dpi theme lvl's we still have incremental jumps. And the current version makes it hard to use bigger textures and just scale them down.
    The only point against it that it is used very little (in games. any editor like app will take full advantage though) and seems like a big change.

Maybe i have to sell it differently:
Instead of thinking of a new theme system think of it that way:
Themes are Classes which give you whatever you need to draw your stuff. You can implement multiple themes with different dpi scales. and than you just change from which theme you get your styles. So why dont we add another class that is smarter. It has ONE additional property (dpi_scale). and based on that it returns the correct sb's textures... For now i just changed the theme class. but it would be possible to also make it a module which provides this coustom theme type.

The other change (Theme props like stylebox icon font constatns) are cached on each control. This is a totally optional feature. I think it is a really good idea since it saves two stacked while loops of hash map lookups which are triggered multiple times per control:
redrawn_control_count * theme_get_sth(string) call count * loop through prent theme that has the style * loop through parent classes which might have the style
Is the amount of hash lookups which need to be done each frame (Only for changed controls.)
will get reduced to:
redrawn_control_count * theme_get_sth(string) call count
for a tiny bit of ram.

Only on theme change the everything gets reloaded. This makes it also possible to do even bigger calculations inside the theme to return generated values.

@Calinou the refactoring is not that big. the theme api stays the same. only the edscale is needs to be replaced with get_gui_base()->get_theme()->get_dpi() we could even just change the macro for EDSCALE ;)

@toger5
Copy link
Contributor Author

toger5 commented May 10, 2019

How would that work?

It seems like it should be a property of control.

@fire It would scale every control in the tree + the dpi scale from that node down.

How do you do the texture and style box scaling?

@fire

  • style box flat: scale every property :) corener radius, border size, margin...
  • style box texture: scale margins, when using the VisualServerDrawNinepatch pass the correct pixel size for the new margins. (ninpatch uses a UV map which gets set up based on pixel size, and margin size)
  • textures: the wrapper textures which are new textures knowing the original one. but pretending to be bigger and draw the new texture bigger (or smaller)

@fire
Copy link
Member

fire commented May 19, 2019

I was able to do this for GraphNodes.

https://github.com/fire/godot/tree/vs_scale

@fire
Copy link
Member

fire commented May 19, 2019

https://www.youtube.com/watch?v=0qZ09Hifbbg

Note that the buttons for plus and minus are buggy at the time of recording but has been improved.

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Nov 7, 2019
@aaronfranke aaronfranke marked this pull request as draft April 9, 2020 00:36
@aaronfranke
Copy link
Member

@toger5 Is this still desired? If so, it needs to be rebased on the latest master branch. Also, since this PR is a feature proposal, you should consider opening a proposal which explains example use cases and how this approach will solve the problem.

Otherwise, abandoned pull requests will be closed in the future as announced here.

@toger5
Copy link
Contributor Author

toger5 commented Jun 16, 2020

I am not up to date with the current gui code anymore. If nothing major has changed I still see it as a nice addition.
I cannot really work on it at the moment. Thanks for notifying that it will get closed. I still might give this another try when i got some ppl convinced it is a nice addition. Dont have the time to defend the idea at the moment. So if someone is intersted in arguing for the feature or even working on it feel free to use it as a resource.

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