-
Notifications
You must be signed in to change notification settings - Fork 73
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
[OoT] OoT Object changes & small changes/fixes/removals #95
Conversation
… ``drawParentSceneRoom`` as it's unused
updated ActorList.xml
# validate the legacy list, if there's any None element then something's wrong | ||
if None in self.ootEnumObjectIDLegacy: | ||
raise PluginError("ERROR: Legacy Object List doesn't match!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note this prevents from removing things from the xml but whatever I guess, can be addressed later if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check makes sense since we want the previous and hardcoded object list, so yea I think it should stay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we only want it in the case the user has old blends, but it'd prevent fast64 from enabling even if they were starting from scratch and just deleted an object in the xml that they also removed in their repo
again this can be addressed later if needed though, just something to be aware of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we had ifdefs in python this wouldn't be an issue 😔
found and fixed a minor issue in |
merged main, is there anything holding this PR from being merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested by exporting a kakariko import and it seems to work fine. I'm not sure exactly where the object list updating is occurring, but I think something needs to communicate to the user how many objects are actually being exported, so they don't get confused when something breaks unexpectedly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. Please merge ASAP.
Tested and it does what it says. I was concerned about support for non-decomp custom actors/objects (which only have a number), but it works and exports them.
One annoyance--when making actors (by adding an empty and changing its type to Actor), or when making other types of objects by this method (even though for scenes and rooms there's an operator available to make them), it says the data is out of date and gives the upgrade button. It works after clicking the button, but things shouldn't be created out of date. Changing the dropdown for the empty type should also do the initialization.
fwiw I don't remember having issues with this PR last time I was actively exchanging about it with Yanis, besides a bit of jank around data upgrading which is tied to the issue mentioned by Sauraen about empty type switching. So once you guys are happy I +1 merging |
I can add an "Add Actor" button in the tools panel but if the user adds an actor manually this issue will stay and idk how to fix that properly |
I've made a comment in discord about a way to fix sauraen's problem with object versioning. Otherwise, I still think there should be something that ensures that the user is aware of when objects are auto-added to the object list (error-checking, warning UI?) especially if those added objects would cause a crash. |
I think I understand how this will fix the issue but I'm not sure how to implement that 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the PR uses the object properties version to handle updates to the OOT object list enum, but its broken currently (#179). The versioning system I suggested in discord also doesn't work, as I realized that old properties that haven't had their default changed are also not accessible in the way I suggested. As a workaround, we can apply the same concept to the objectID
property on the OOTObjectProperty
itself. Remove the objectID
definition, then in code the presence of objProp["objectID"]
indicates an un-upgraded property. The property can be updated, then del objProp["objectID"]
is called. The exporter can handle both upgraded and un-upgraded versions using this check, so that even if the user doesn't update the export will still be correct.
I understand how your idea is working but I don't know how to implement that |
Updated the upgrade system with Kure's suggestions, also I thought it'd be fine if I removed the version system completely since the property trick works better, now we just need to figure out a standard way to do this but it can be done later Changed the UI to make it draw, if it's still using old data everything on the object list will be disabled but visible with the upgrade button at the top, also exporting should work properly if the blend isn't updated yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Will merge soon if unless anyone objects.
List of changes:
ObjectList.xml
which contains every objects from OoT, you have the name, the decomp ID and something calledObjectKey
, it's used to identify an object, this should never change (the format isobj_
+ the debug name of the object)ActorList.xml
, currently unused outside identifying an actor, it will be used whenever PR 56 is merged though, the format of this file is more complex because there's every parameters etc, but it's a bit like the object list file\t
in OoT files with theindent
variable introduced with PR 79 (decomp uses 4 spaces)oot_parse.py
andsceneDirectoryParser.py
as it's unusedActorList.xml
file being here), it's also adding it in the UITL;DR: new features regarding OoT objects and some fixes/removed unused stuff/enhanced the C output a bit
Object list preview
Actor/Object Lists defines preview