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

Clarify relationship between basis and transform properties of Node3D #80254

Merged

Conversation

marcospb19
Copy link
Contributor

I'm confused about which Transform3D the Node3D.basis docs refer to:

  • self.transform.basis, or
  • self.global_transform.basis

This PR edits the Node3D.basis description to clarify this, although I'm not sure I guessed the right one.

@marcospb19 marcospb19 requested a review from a team as a code owner August 4, 2023 14:45
@marcospb19 marcospb19 changed the title gdscript docs: edit description of Node3D.basis gdscript docs: edit description of Node3D.basis Aug 4, 2023
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
@YuriSizov YuriSizov changed the title gdscript docs: edit description of Node3D.basis Clarify relationship between basis and transform properties of Node3D Aug 4, 2023
@YuriSizov YuriSizov added this to the 4.2 milestone Aug 4, 2023
@fire fire requested review from a team August 4, 2023 15:01
@fire
Copy link
Member

fire commented Aug 4, 2023

The Transform3D can be in more global space (it's not global to the root if it is outside the tree.) or can be in local space.

The global transform calculation does caching because the ordinary use requires going through all the parent Node3D global_transforms, getting their transforms and then concatenating them.

The 3x4 Transform3D matrix has an Vector3 origin and a 3x3 basis matrix.

@marcospb19
Copy link
Contributor Author

@fire

The Transform3D can be in more global space (it's not global to the root if it is outside the tree.) or can be in local space.

The global transform calculation does caching because the ordinary use requires going through all the parent Node3D global_transforms, getting their transforms and then concatenating them.

The 3x4 Transform3D matrix has an Vector3 origin and a 3x3 basis matrix.

Thanks for the extra explanation on caching.


The doubt I had was: if .basis is a shorthand for a Transform3D property, and this class has two Transform3D properties, then the docs should be more specific about which one it points to.

godot/scene/3d/node_3d.cpp

Lines 295 to 298 in cc6a609

Basis Node3D::get_basis() const {
ERR_READ_THREAD_GUARD_V(Basis());
return get_transform().basis;
}

Found this code, so it's the non-global one.

@YuriSizov I applied the suggested change and force-pushed.

@marcospb19 marcospb19 force-pushed the edit-description-of-node3d-basis branch from eb66864 to 07fd89b Compare August 4, 2023 15:29
@YuriSizov
Copy link
Contributor

Sorry, I should've mentioned it from the getgo, but can you please amend your commit message. It doesn't really follow our preferred style. You can use the current title of the PR, for instance.

@akien-mga akien-mga force-pushed the edit-description-of-node3d-basis branch from 07fd89b to 9e6da4e Compare August 7, 2023 10:13
@akien-mga
Copy link
Member

Sorry, I should've mentioned it from the getgo, but can you please amend your commit message. It doesn't really follow our preferred style. You can use the current title of the PR, for instance.

Did the change myself in the end.

@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 7, 2023
@akien-mga akien-mga merged commit 5146096 into godotengine:master Aug 7, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@marcospb19 marcospb19 deleted the edit-description-of-node3d-basis branch August 7, 2023 14:40
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
@YuriSizov
Copy link
Contributor

It's a nice clarification, but I'll skip it from cherry-picking so we don't invalidate existing translations.

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.

4 participants