[OoT] New Exporter

[Yanis42]

This is something I wanted to do for a while, it should work properly according to my tests (thanks again to Richie for that)

This relies on dataclasses, I don't think it's the best thing for performances but at least it makes the code easy to follow imo, also it includes functions to get C data so it's kinda 2 in 1, this makes you able to call one of the classes to export a specific type of data, I did that for cutscenes and collisions (collisions required more changes but cutscenes are a one-line change)

Anyway this was fun to make!

---

Any feedback/suggestions is appreciated! (the diff is huge but this doesn't edit a lot of the original files, most of the diff comes from new files I created)

Extra note: this fix the known issues related to waterbox duplicates, paths and the file(s) being inconsistent at export time, for instance actor entries swapping in the actor list for no reasons

---

This exporter has everything in the same place (inside the exporter folder) instead of having bits of export scattered here and there, I think it makes more sense like this

[Yanis42]

This should be ready to review now, I don't think I'm gonna add something else (it's already huge sorry)
I did almost everything on my todo list, the only remaning item is removing the older code

I decided to keep the checkbox I made to toggle between the two versions, I simply commented the lines, this is on purpose and will be removed with the old version (I can remove that once the review is done and ready to merge of course)

[kurethedead]

Overall I like the organization into more smaller modular parts, but I have a couple issues. Note that I mostly skimmed through the PR, I didn't test it for correctness yet but mostly just for code organization. I didn't mark all of the spots where the following comments apply, just some of them. I'm also a little rusty on OOT stuff so let me know if some of the comments are wrong.

1. A lot of the classes marked with @dataclass shouldn't be dataclasses at all, assuming they're marked as such in order to handle automatic __init__() generation. A lot of init-related temporary state is unnecessarily stored as variables, and a lot of derived members are given optional values (where they shouldn't be) in order to "hide" them from being needed in the __init__(). The use of __post_init__() seems to be a workaround to getting derived state, but at that point its much more clear to just define an __init__() function that only takes in the necessary arguments.
2. A couple classes are just collections of static methods - they should just be functions imported from a separate file, or the methods should be marked @staticmethod to avoid class instantiation. For example:

from . import spec
...
spec.editSpec(self.exporter)

[kurethedead]

These should not be optional?

[Yanis42]

I know it's weird but I'd like to keep this initialised to None so if there's any uncaught/unknown issue it would make it more obvious what the issue is, and just using Optional since I was told thing: str = None is bad

[kurethedead]

Wouldn't it be easier to catch errors at construction time of the TransitionActor instead of letting this continue on with invalid state?

[Yanis42]

I think I can consider this as solved too (field(init=False)), the members are set in the same function as the one that calls this class in the first place (which is SceneTransitionActors's post init)

[kurethedead]

This should be converted to a file with imported functions or a class with static methods that can be used by other classes.

[Yanis42]

I renamed it "Utility", let me know if you think of a better name (and I used static methods)

[Dragorn421]

Fwiw I don't know the pythonic way to have "dataclasses but some of the fields will be initialized later"

I'll try to look it up

[Yanis42]

(just in case I forget: https://discord.com/channels/874816709855440926/874819026629574717/1200770808927244499)

[kurethedead]

Also, I'm a bit confused about the overall file organization at this point - is the new exporter folder going to replace all the scene/room/etc. related exporter files? Is DL/skeleton/animation/collision exporting going to be a part of the exporter folder or will it be separate?

[Yanis42]

Also, I'm a bit confused about the overall file organization at this point - is the new exporter folder going to replace all the scene/room/etc. related exporter files? Is DL/skeleton/animation/collision exporting going to be a part of the exporter folder or will it be separate?

for now they will stay because it would make this PR way too big (and painful for conflicts) but I want to remove everything called when you use ootExportSceneToC (basically it will remove oot_level_writer.py and scene/exporter/to_c/ if this gets merged

and of course I will remove the related files to the non-scene exporters I implemented

[kurethedead]

In regards to optional arguments and overriding (#256 (comment)), what I meant is that someone could write the code Cutscene(csObj, true, frameCount = 100). However __post_init__ would immediately override that frameCount, which would not only be unintended behaviour - it wouldn't even throw an error at that point, which could be a source of headaches. Using field(init=False) does fix this, but doesn't address the other issues I have with the current dataclass usage.

I think there's a discrepancy about how we're looking at dataclasses. From what I'm understanding, dataclasses are supposed to be for classes that are primarily for storing state. Using it for the f3d command classes makes sense in that case, as those only really store state and are operated on by other functions/classes. However, in a lot of cases the export dataclasses have the export functionality embedded in them, which leads to a lot of questionable design decisions.

1. Constructor-specific arguments (including blender objects) being stored as members of the dataclass. I think in almost all cases blender objects shouldn't be members on these classes, as the classes are supposed to be pure data representations of blender objects/properties already. As for non-blender object arguments, an example of confusing design choices is with CutsceneCmdBase and its subclasses, particularly with the way params and paramNumber is handled in order to fit it into the dataclass syntax.

   • params, which should only appear in the child class's __init__, is now defined as a member in the base class and is queried in all child classes.
   • paramNumber, which should be defined in the base class, is separately defined in every child class.

2. Using field(init=False) on almost all arguments, then setting all the arguments either externally, or from other constructor arguments. The fact that this has to be done kind of defeats the whole purpose of using dataclasses for __init__ generation. The former case in particular basically makes all arguments effectively optional, which they shouldn't be.

I think a good middle ground would be a solution like this: https://stackoverflow.com/a/72519184. Basically, the dataclass doesn't use any field(init=False), which indicates that the object should be constructed with all arguments provided and no optional values. Then we have a @class_method that constructs the object based on constructor-only arguments (ex. blender objects), which we no longer have to store as members, instead of using __post_init__. This would also fix other design choices made to circumvent dataclass functionality (ex. paramNumber can be defined only once in CutsceneCmdBase)

[kurethedead]

Also, are all the cutscene commands defined twice in different files but with identical definitions?

[Yanis42]

Also, are all the cutscene commands defined twice in different files but with identical definitions?

I wanna do this change in two rounds (or more if needed), round 1 the refactored exporter (this PR), round 2 remove all of the unused code and if that's too long round 3 adapt the remaining cutscene stuff to this, which is the cutscene importer

So yea I know it's not ideal but I'm concerned about PR sizes

[Yanis42]

In regards to optional arguments and overriding (#256 (comment)), what I meant is that someone could write the code Cutscene(csObj, true, frameCount = 100). However __post_init__ would immediately override that frameCount, which would not only be unintended behaviour - it wouldn't even throw an error at that point, which could be a source of headaches. Using field(init=False) does fix this, but doesn't address the other issues I have with the current dataclass usage.

I think there's a discrepancy about how we're looking at dataclasses. From what I'm understanding, dataclasses are supposed to be for classes that are primarily for storing state. Using it for the f3d command classes makes sense in that case, as those only really store state and are operated on by other functions/classes. However, in a lot of cases the export dataclasses have the export functionality embedded in them, which leads to a lot of questionable design decisions.

1. Constructor-specific arguments (including blender objects) being stored as members of the dataclass. I think in almost all cases blender objects shouldn't be members on these classes, as the classes are supposed to be pure data representations of blender objects/properties already. As for non-blender object arguments, an example of confusing design choices is with `CutsceneCmdBase` and its subclasses, particularly with the way `params` and `paramNumber` is handled in order to fit it into the dataclass syntax.
   
   * `params`, which should only appear in the child class's `__init__`, is now defined as a member in the base class and is queried in all child classes.
   * `paramNumber`, which should be defined in the base class, is separately defined in every child class.

2. Using `field(init=False)` on almost all arguments, then setting all the arguments either externally, or from other constructor arguments. The fact that this has to be done kind of defeats the whole purpose of using dataclasses for `__init__` generation. The former case in particular basically makes all arguments effectively optional, which they shouldn't be.

I think a good middle ground would be a solution like this: https://stackoverflow.com/a/72519184. Basically, the dataclass doesn't use any field(init=False), which indicates that the object should be constructed with all arguments provided and no optional values. Then we have a @class_method that constructs the object based on constructor-only arguments (ex. blender objects), which we no longer have to store as members, instead of using __post_init__. This would also fix other design choices made to circumvent dataclass functionality (ex. paramNumber can be defined only once in CutsceneCmdBase)

so basically you want me to revert the 1 hour and a half-ish of work I did yesterday to use field(init=False) (idr how long but it felt long), also I still don't understand why this is a bad thing, I read the proposal and the documentation yesterday and my conclusion was dataclasses are meant to be used to make the code prettier, atm everything works exactly as if it wasn't dataclasses so I'm highly confused about why this is bad

it seems to me it's down to what we want in the codebase and not what Python tells us to do, I'm just tired of changing the ~50 different classes this PR adds + testing everything works, fixing issues etc, I usually spent a whole day doing everything and I just wanna move on something else at this point, and I already spent a lot of time working on this (since september, so like 5 months)

btw the whole point of this system is:

1. instanciate a class with the required parameters
2. ready to export

and I feel what you want just breaks this entirely idk, it makes me feel like all of the time I spent on this was basically for nothing lol

I'd like to hear what @Dragorn421 thinks of this and if he got other ideas for a solution before doing anything else, I don't wanna do lots of changes again if it turns out it wasn't necessary

[kurethedead]

I'm sorry about the extra work, but I did state that I thought init=False was not a good fit for the issues at hand, so I'm not sure why you went ahead with that without more discussion. I think you're missing the main point, which is that there are other issues related to the current code organization besides init=False that you aren't addressing. What I'm describing is a code debt issue, not a style issue. The fix I suggested wouldn't break the system you're describing at all, its still the same basic idea.

[Yanis42]

I'll try to do the changes soon, I think it's dumb that my own opinions are blocking this cool thing