search

rojo-rbx / rojo

Rojo enables Roblox developers to use professional-grade software engineering tools
https://rojo.space
Mozilla Public License 2.0
908 stars 172 forks source link

Insert Model.NeedsPivotMigration in insert_instance when missing #865

Closed kennethloeffler closed 4 months ago

kennethloeffler commented 4 months ago

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)

Dekkonot commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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