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

Insert Model.NeedsPivotMigration in insert_instance when missing #865

Merged

Conversation

kennethloeffler
Copy link
Member

This PR closes #628 by adding a change to the 7.4.x branch that inserts Model.NeedsPivotMigration when it's missing from any instance that inherits from Model.

This is a pretty poor solution, but this bug needs to be fixed for 7.4.1 and the scope of the work in rojo-rbx/rbx-dom#385 (which when closed, will properly fix the underlying problem) is a little too large to comfortably fit into 7.4.1.

Please make sure I got all the classes, and of course test the changes (I did confirm that they work myself, but it's good to have more sets of eyes)

@kennethloeffler kennethloeffler added scope: cli Relevant to the Rojo CLI size: small impact: medium Moderate issue for Rojo users or a large issue with a reasonable workaround. version 7.4 labels Feb 16, 2024
Dekkonot
Dekkonot previously approved these changes Feb 19, 2024
Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

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

Going to hesitantly approve this because we need to fix this. I don't like it though.

@Dekkonot Dekkonot dismissed their stale review February 19, 2024 16:30

Double checked and unable to observe fix in test model

@Dekkonot
Copy link
Member

Dekkonot commented Feb 19, 2024

Actually upon further inspection I'm not actually able to tell if this has worked. The test model in #628 looks to be identical in both versions, but it's possible I am missing something.

What test were you using to check this?

EDIT: After using the test file you gave in #866, it seems to work. I wonder if the test file provided by the original issue was flawed in some way?

@kennethloeffler
Copy link
Member Author

Actually upon further inspection I'm not actually able to tell if this has worked. The test model in #628 looks to be identical in both versions, but it's possible I am missing something.

What test were you using to check this?

EDIT: After using the test file you gave in #866, it seems to work. I wonder if the test file provided by the original issue was flawed in some way?

It does work for the first issue - the pivot in the first issue's repro is positioned at the bottom:
Screenshot 2024-02-19 at 2 25 05 PM

@Dekkonot
Copy link
Member

...So it is. Sometimes I wonder about myself. Alrighty, nevermind then!

@Dekkonot Dekkonot merged commit e23d024 into rojo-rbx:7.4.x Feb 20, 2024
@kennethloeffler kennethloeffler deleted the quick-hack-needs-pivot-migration branch February 20, 2024 17:52
Dekkonot added a commit to UpliftGames/rojo that referenced this pull request Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: medium Moderate issue for Rojo users or a large issue with a reasonable workaround. scope: cli Relevant to the Rojo CLI size: small version 7.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants