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

Rename Vector4.components -> coord for consistency #97487

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Sep 26, 2024

This brings it in line with all the other vectors:

I'm guessing this naming happened when Vector4 was copied from Color, as it's essentially the same struct layout, just with different functions.

This is obviously a potentially breaking change. I didn't find any other references in core to 'components', though at least godot-cpp has named their Vector4.components property after this one (see godotengine/godot-cpp#1608), so godot-cpp users may be affected.

@AThousandShips
Copy link
Member

AThousandShips commented Sep 26, 2024

For the engine itself this isn't compatibility breaking as we don't provide compatibility guarantees for the engine internals (only the exposed API)

Adding a review request for GDExtension as this is related to the godot-cpp side PR and is relevant for deciding either way

@AThousandShips AThousandShips requested a review from a team September 26, 2024 12:07
@akien-mga akien-mga requested a review from a team September 26, 2024 12:32
@dsnopek
Copy link
Contributor

dsnopek commented Sep 26, 2024

From the perspective of godot-cpp, if we want to maintain source compatibility, we could keep both declarations in the union and add a note about the older one being deprecated. Vector4 is new enough, though, that I think an argument could be made that we don't need to do that.

In any case, I think it's fine to move forward with this on the Godot-side, and we can have the "maintain source compatibility for Vector4.components or not" discussion in the follow-up godot-cpp PR

@akien-mga akien-mga changed the title Rename Vector4.components -> coord Rename Vector4.components -> coord for consistency Sep 26, 2024
@fire
Copy link
Member

fire commented Sep 26, 2024

We tend to use full names so instead of coord -> coordinates, but this was an internal that was exposed.. Not sure what to do.

Edited:

At this point adding components to all the other types is also a valid diff.

@Ivorforce
Copy link
Contributor Author

Ivorforce commented Sep 26, 2024

I'd be ok with switching to coordinates for all the types too, I'm not a big fan of shorthands. But I'll let reviewers decide.

@akien-mga
Copy link
Member

I think this change is fine as is, I wouldn't change all Vector types further. If we're to change I'd go one step further and make this a private member so we don't even need to bother about its name or compatibility, but that'd be a Godot 5 change :)

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Sep 26, 2024
@akien-mga akien-mga merged commit 8126d81 into godotengine:master Sep 26, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@Ivorforce Ivorforce deleted the patch-2 branch September 26, 2024 17:46
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