godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.38k stars 21.26k forks source link

Replace hardcoded radial center cross marker for `TextureProgressBar` with internal `Marker2D` child node #99628

Closed arkology closed 5 hours ago

arkology commented 1 day ago

Updated after separating 1) replacing hardcoded marker and 2) issues with its visibility in plugins.

This is proof of concept showing how https://github.com/godotengine/godot-proposals/issues/11225 may be implemented, nothing more.

How it looks: godot windows editor dev x86_64_RiIircpkeu Current PR
1024x1024 Godot_v4 4-dev2_win64_Lksr6p93Qr godot windows editor dev x86_64_PLyyQBxnf3
48x48 Godot_v4 4-dev2_win64_duNiAhNCOF godot windows editor dev x86_64_6ZL3ie810C

Pros: 1) Crosshair now doesn't look unnatural compared to other stuff in editor. 2) Reusing existing functionality of Marker2D node. 3) Allows seeing actual center more precisely - may be good for low-res art.

Cons: 1) Additional overhead in editor as additional node is created. 2) Less code readability, too overcomplicated.

Implementation details:

Mickeon commented 1 day ago

I'm not... necessarily against this but it would be a whole lot less code to just draw what the Marker2D node draws, wouldn't it? This feels over-engineered. Even the additional note in the docs is over-engineered.

arkology commented 1 day ago

The simplest way will be just to add property to show/hide this reference cross. But this reference cross will stay hardcoded (which is not good IMO) and this property will be present in release builds (please correct me if there is possibility to make editor-only properties).

I'm not saying that current PR is the best that could be done, it's just possible option. First of all this is just concept of how it may be changed to gather any feedback. Most likely this PR will be stalled (2 issues in ~7 years - not high-priority problem at all) and I'll close it later.

Upd: I've added possible alternatives into feature proposal

Mickeon commented 1 day ago

and this property will be present in release builds (please correct me if there is possibility to make editor-only properties).

There are precedents. For example: https://github.com/godotengine/godot/blob/0c45ace151f25de2ca54fe7a46b6f077be32ba6f/scene/main/scene_tree.h#L140-L144 https://github.com/godotengine/godot/blob/0c45ace151f25de2ca54fe7a46b6f077be32ba6f/scene/main/scene_tree.h#L350-L368

Notice how in every other build other than debug the getters always return false (which the compiler likely optimises out). Yeah, the property should still be bound, but its getter and setters can be coded to do nothing.

I do find that reusing Marker2D for this is kind of clever but my personal concern still stands.

Mickeon commented 1 day ago

Also, wait. Wouldn't Node.is_part_of_edited_scene() also solve the original issue?

arkology commented 1 day ago

Also, wait. Wouldn't Node.is_part_of_edited_scene() also solve the original issue?

I'll try this method, thanks!

arkology commented 6 hours ago

@Mickeon thank you so much for referencing to Node.is_part_of_edited_scene(). This solves issue as expected. godot windows editor dev x86_64_frtvhxH0pr

arkology commented 6 hours ago

So, this PR is now just showcase for replacing hardcoded reference cross with Marker2D (i.e. possible solution for feature request), nothing more. This one will not close (is not targeting to close) mentioned issues with reference cross. I'll open separate PR to replace Engine::get_singleton()->is_editor_hint() with is_part_of_edited_scene() which will close original issues. And then update first comment of this PR with some gifs and then close it. Sounds like a plan.

arkology commented 5 hours ago

Updated OP with some clarifications and gif, closing now. Still could be tested (at least for now) by downloading PR artifacts if anyone is interested (click Commits -> Artifacts)