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.

Sign up for GitHub

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

Jump to bottom

Remove the need to restart editor when modifying most editors/3d_gizmos settings #101920

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

Remove the need to restart editor when modifying most editors/3d_gizmos settings #101920

wants to merge 1 commit into from
+221 −134

Conversation

yahkr
Copy link
Contributor

@yahkr yahkr commented Jan 22, 2025 •
edited
Loading

This PR improves the editor by not requiring a restart to change 3d gizmo colors/settings
I've tested each of the related gizmos and all appear to update without issue.

image

Notes for the future:

1. The following colors are unused currently: see #101920 (comment) and #101920 (comment)

editors/3d_gizmos/gizmo_colors/spring_bone_joint
editors/3d_gizmos/gizmo_colors/spring_bone_collision
editors/3d_gizmos/gizmo_colors/spring_bone_inside_collision
editors/3d_gizmos/gizmo_colors/path
editors/3d_gizmos/gizmo_colors/particles_emission_shape
  1. editors/3d_gizmos/gizmo_settings/path3d_tilt_disk_size wasn't able to be changed in a similar way because it instantiates a Path3DGizmo with a specified disk size.

@yahkr yahkr requested review from a team as code owners January 22, 2025 16:57
@yahkr yahkr mentioned this pull request Jan 22, 2025
Open
@Calinou Calinou added this to the 4.x milestone Jan 22, 2025
Calinou
Calinou reviewed Jan 22, 2025
View reviewed changes
editor/editor_settings.cpp Outdated Show resolved Hide resolved
Calinou
Calinou reviewed Jan 22, 2025
View reviewed changes
editor/plugins/gizmos/decal_gizmo_plugin.cpp
create_material("decal_material", gizmo_color);

create_handle_material("handles");
DecalGizmoPlugin::update_materials();
Copy link
Member

@Calinou Calinou Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The class qualifier is redundant as far as I can tell:

Suggested change
DecalGizmoPlugin::update_materials();
update_materials();

Same in the other gizmo classes.

Copy link
Contributor Author

@yahkr yahkr Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Rider was upset because we're calling a virtual function within the constructor. Still don't want the qualifier?

Calinou
Calinou reviewed Jan 22, 2025
View reviewed changes
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.

I would also change the GridMap gizmo code added by #101101 to make use of this new update_materials() function.

@yahkr
Copy link
Contributor Author

yahkr commented Jan 22, 2025 •
edited
Loading

I would also change the GridMap gizmo code added by #101101 to make use of this new update_materials() function.

So it looks like Gridmap handles this via notifications and doesnt have access to the update_materials() function from EditorNode3DGizmoPlugin

godot/modules/gridmap/editor/grid_map_editor_plugin.cpp

Lines 1255 to 1256 in f1c0b5b

case EditorSettings::NOTIFICATION_EDITOR_SETTINGS_CHANGED: {
indicator_mat->set_albedo(EDITOR_GET("editors/3d_gizmos/gizmo_colors/gridmap_grid"));

Edit: just to clarify, the Gridmap setting does work with current PR, its just not visible in the image i included beyond the scene tree

@yahkr yahkr force-pushed the fix_gizmo_restart branch from 5ef1773 to 796153f Compare January 22, 2025 17:19
@RedMser
Copy link
Contributor

RedMser commented Jan 22, 2025 •
edited
Loading

The following colors are unused currently:

editors/3d_gizmos/gizmo_colors/spring_bone_joint
editors/3d_gizmos/gizmo_colors/spring_bone_collision
editors/3d_gizmos/gizmo_colors/spring_bone_inside_collision
editors/3d_gizmos/gizmo_colors/path
editors/3d_gizmos/gizmo_colors/particles_emission_shape

While you're correct that path seems unused, I found

@yahkr yahkr force-pushed the fix_gizmo_restart branch from 796153f to aaf6c97 Compare January 23, 2025 12:27
@yahkr
Copy link
Contributor Author

yahkr commented Jan 23, 2025

While you're correct that path seems unused, I found
...
editor\plugins\gizmos\spring_bone_3d_gizmo_plugin.cpp:110
editor\plugins\gizmos\spring_bone_3d_gizmo_plugin.cpp:265
...
editor\plugins\gizmos\particles_3d_emission_shape_gizmo_plugin.cpp:46

Much appreciated, the first two require no changes, so just the third. It's odd they are excluded from the build within my .vcxproj
Perhaps because the file is so new? Hard for me to tell based on file history

In either case I've updated particles_3d_emission_shape_gizmo_plugin.cpp anyway and pushed this up :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Calinou Calinou Calinou left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement topic:editor topic:3d
Projects
None yet
Milestone
4.x
Development

Successfully merging this pull request may close these issues.

Remove the need to restart editor when modifying most 3D Gizmos settings
3 participants
@yahkr @RedMser @Calinou