Skip to content

Fix #713: rename ControlerHider → ControllerHider#738

Closed
Suriya-Palaniswami wants to merge 2 commits into
GodotVR:masterfrom
Suriya-Palaniswami:master
Closed

Fix #713: rename ControlerHider → ControllerHider#738
Suriya-Palaniswami wants to merge 2 commits into
GodotVR:masterfrom
Suriya-Palaniswami:master

Conversation

@Suriya-Palaniswami

Copy link
Copy Markdown
Contributor

Renamed ControlerHider to Controller Hider

Tested this by importing it to Godot 4.3. No output errors and the editor compiles successfully.

XRToolsDesktopControllerHider is compiled as seen in the screenshot.

godot#173

@BastiaanOlij

Copy link
Copy Markdown
Member

Would need to test if this causes a backwards compatibility issue for project already using this, but I don't think so as the file name doesn't change.

Would be nice if some of the accidental changes could be filtered out (changes to the action map and positioning in the main level scene). But that's probably not that easy so more of a nitpick.

@BastiaanOlij BastiaanOlij added the cleanup Cleaning up code or aligning Godot 3 and 4 versions label Jul 29, 2025

@BastiaanOlij BastiaanOlij left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is ok but would like a second opinion from @pietru2004

@pietru2004

Copy link
Copy Markdown
Contributor

I don't see a problem, but please keep in mind that that it should be probably marked in red in changelog or smthg like that since class name changes here.

Also this one will probably only be important to remember for those that use desktop support.

@pietru2004

Copy link
Copy Markdown
Contributor

also is there difference between controler and controller ?

@Suriya-Palaniswami

Copy link
Copy Markdown
Contributor Author

@pietru2004 Sorry, I am new here. Not sure if I should be the one updating the changelog? And to answer your question, there is no difference between controler and controller. It was just misspelled.

@pietru2004

Copy link
Copy Markdown
Contributor

to be honest @Suriya-Palaniswami IDK should you be updating changelog, I assume the core maintainers will do it before release or smthg like that

@Suriya-Palaniswami

Copy link
Copy Markdown
Contributor Author

Makes sense @pietru2004, thanks for clarifying. Considering that checks have passed. I think this is ready to merge. I'll leave this for the maintainers as I don't have permissions. And then subsequently the issue can be closed as well.

@BastiaanOlij

Copy link
Copy Markdown
Member

We try and update the change log on release, but if its added to the PR than that just takes away the effort :)

@BastiaanOlij

Copy link
Copy Markdown
Member

@pietru2004 controller is correct spelling I think, its mainly just that renaming classes does tend to be a breaking change. So it's important to ask if this will be a problem or is acceptable seeing you originally wrote the implementation.

I'm happy to merge this as is.

@pietru2004

Copy link
Copy Markdown
Contributor

@BastiaanOlij I would say it is an important node, it basicaly hides controlers if desktop mode is active atm, but as long as it is marked in red or as possibly breaking change in changelog I think it should be good.

@BastiaanOlij

Copy link
Copy Markdown
Member

@Suriya-Palaniswami if you don't mind giving this a quick rebase I'll merge it in.

@BastiaanOlij

Copy link
Copy Markdown
Member

I've rebased this into #757 and merged, thanks!

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

Labels

cleanup Cleaning up code or aligning Godot 3 and 4 versions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants