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

Expose Node3D.scale() as scale_object() #90321

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

Conversation

Muller-Castro
Copy link
Contributor

@Muller-Castro Muller-Castro commented Apr 6, 2024

Expose Node3D.scale() to the editor side as scale_object().

@Muller-Castro Muller-Castro requested review from a team as code owners April 6, 2024 19:25
@akien-mga akien-mga changed the title Add scale method to the editor Expose the Node3D.scale() method Apr 6, 2024
@akien-mga akien-mga added this to the 4.x milestone Apr 6, 2024
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

This isn't valid, you can't expose scale because there's already the scale property

@Muller-Castro
Copy link
Contributor Author

This isn't valid, you can't expose scale because there's already the scale property

Can this be exposed under another name?

@AThousandShips
Copy link
Member

Sure, unsure what name would be appropriate though

Also unsure if we should have a whole three different scaling methods, we already have global_scale and scale_object_local

@Muller-Castro
Copy link
Contributor Author

If I haven't tested it wrong, this method scales along the local axes of the parent node so it's different from the other two.

@Muller-Castro
Copy link
Contributor Author

Here's the difference:

Example

@AThousandShips
Copy link
Member

AThousandShips commented Apr 6, 2024

Right, it's different, I'm saying it'll be confusing with three different ones

@Muller-Castro
Copy link
Contributor Author

It'd pair with rotation methods as there are 3 different types rotate_object_local, rotate and global_rotate.

In the end this is just a shortcut for transform.basis = transform.basis.scaled(Vector3(x, y, z))

@Muller-Castro
Copy link
Contributor Author

Sorry I'm confused. Should I close this PR or change the exposed name?

@AThousandShips
Copy link
Member

Go ahead and change it and we'll see what name is agreed on

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

We resolved the name overlap so I think the scale_object gdscript expose is fine.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Let's add the details for this one in #87440

@Mickeon
Copy link
Contributor

Mickeon commented Apr 10, 2024

Commit & PR could use a better title since the name has changed

@Muller-Castro
Copy link
Contributor Author

Such as "Expose Node3D.scale() as scale_object()" ?

@Mickeon
Copy link
Contributor

Mickeon commented Apr 10, 2024

Yes, sounds fine enough

@Muller-Castro Muller-Castro changed the title Expose the Node3D.scale() method Expose Node3D.scale() as scale_object() Apr 10, 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.

Docs are consistent and just fine, if this review is of any additional support.

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.

5 participants