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

Fix example for Basis * Vector3 in documentation #92117

Merged

Conversation

mart3323
Copy link
Contributor

@mart3323 mart3323 commented May 19, 2024

Description

This MR fixes an invalid example in the Basis documentation, fixes godotengine/godot-docs#9394

I opted to change the inputs instead of the outputs, because it seemed strange to have two identical basis vectors. Plus this allows some of the inputs to be negative

Actions performed

  • Cloned both projects, symlinked them together and ran the build both before and after the changes to confirm they show up where expected
  • Retested the GDScript example in editor to confirm the new example gives the promised result
  • ⚠️ Did not test the C# example. Though I have no reason to believe it should run any different
  • Checked the original MR to see if there were any comments about this example to be aware of
  • Ran pre-commit hooks (4 passed, rest skipped)

@mart3323 mart3323 requested a review from a team as a code owner May 19, 2024 11:57
@AThousandShips AThousandShips added bug documentation cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels May 19, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone May 19, 2024
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Although I'd be more in favour of correcting the "Prints" statement, this is correct, too. Thank you for cleaning after me.

doc/classes/Basis.xml Outdated Show resolved Hide resolved
@mart3323
Copy link
Contributor Author

mart3323 commented Jun 9, 2024

Although I'd be more in favour of correcting the "Prints" statement, this is correct, too. Thank you for cleaning after me.

Any specific reason?
If desired, I have no problem changing it - either to the original

  var my_basis = Basis(Vector3(1, 1, 1), Vector3(1, 1, 1), Vector3(0, 2, 5))
  print(my_basis * Vector3(1, 2, 3)) # Prints (3, 9, 18)

or a hybrid that just barely changes the inputs to make them different

  var my_basis = Basis(Vector3(1, 1, 1), Vector3(1, -1, 1), Vector3(0, 2, 5))
  print(my_basis * Vector3(1, 2, 3)) # Prints (3, 5, 18)

or to some new example like

  # Basis that swaps the x/z axis and doubles the scale
  var my_basis = Basis(Vector3(0, 2, 0), Vector3(2, 0, 0), Vector3(0, 0, 2))
  print(my_basis * Vector3(1,2,3)) # Prints (4, 2, 6)

@Mickeon
Copy link
Contributor

Mickeon commented Jun 9, 2024

It is mostly to keep the numbers simpler and easier to digest (although a transformation is anything but simple at a glance).

I like the new example you proposed.

@mart3323 mart3323 force-pushed the godot-docs-basis-op-mul-vector branch from 4e477c0 to 8862e25 Compare June 9, 2024 17:41
doc/classes/Basis.xml Outdated Show resolved Hide resolved
doc/classes/Basis.xml Outdated Show resolved Hide resolved
@akien-mga akien-mga force-pushed the godot-docs-basis-op-mul-vector branch from 8862e25 to 771f52e Compare June 10, 2024 11:31
@akien-mga
Copy link
Member

I rebased the PR to fix the (temporary) CI issue, and did the couple tweaks I had suggested while at it.

@akien-mga akien-mga merged commit 3d9ee2d into godotengine:master Jun 10, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Example for (Basis * Vector3) is incorrect
5 participants