oead 1.2.5 & 1.2.5.post1- BCML issues

[ghost]

The following versions completely break Linkle 3.0.1 and Girly Animation Pack specifically, there are other mods that just hang on the title screen also for wiiu/switch users. Downgrading to oead 1.2.4.post2 fixes these issues and allows said mods to work flawlessly.

[leoetlino]

There's not enough actionable information for me to debug the issue. I don't have time to set up downstream software and try mods on a Switch.

Please provide a Minimal Reproducible Example showing an actual bug in oead (e.g. BYML dicts not behaving as expected), or ask someone who can do that.

[HGStone]

this may or may not be helpful, but I ran a diff on the 2 BCML outputs using the Linkle mod, there was only 1 file that was different between the 2, here is the Player.baslist from each merge using the 2 different versions oead,
Player.baslist.zip

[leoetlino]

Looks like some of the ASDefines get merged incorrectly, and some of the file names are wrong?

-        ASDefine_247: !obj {Name: !str64 MotorcycleDashWait, Filename: !str64 Player_MotorcycleDashWait}
-        ASDefine_248: !obj {Name: !str64 MotorcycleDrift, Filename: !str64 Player_MotorcycleDrift}
-        ASDefine_249: !obj {Name: !str64 MotorcycleFall, Filename: !str64 Player_MotorcycleFall}
-        ASDefine_250: !obj {Name: !str64 MotorcycleLand, Filename: !str64 Player_MotorcycleLand}
+        ASDefine_247: !obj {Name: !str64 MotorcycleDashWait, Filename: !str64 MotorcycleDashWait}
+        ASDefine_248: !obj {Name: !str64 MotorcycleDrift, Filename: !str64 MotorcycleDrift}
+        ASDefine_249: !obj {Name: !str64 MotorcycleFall, Filename: !str64 MotorcycleFall}
+        ASDefine_250: !obj {Name: !str64 MotorcycleLand, Filename: !str64 MotorcycleLand}

Theoretically this gives us a lead for debugging but given that downstream is switching to a Rust fork of oead soon (iirc) I'm not sure it's worth spending too much of my time on this especially when I am unfamiliar with the merger code.

[GingerAvalanche]

I've not been able to recreate this outside of BCML, but the only part of BCML that edits this file for these mods doesn't change the content of the ASDefines at all, so I'm pretty stumped about where the faulty code could be. Maybe the following might give you an idea.

BCML converts both the vanilla ASDefines ParameterList and the mod one to Dict[str, str], overwrites the vanilla's values for each key with the mod's, and converts the result back to ParameterList. If there's no changes in the mod, it converts out and then immediately converts back.

Neither Linkle 3.0 nor Girly Animation Pack edit the ASDefines, so for the mentioned mods, it just converts to Dict then immediately back to ParameterList. Running this exact same routine outside of BCML never produces an error; it's a newly-created ParameterList, but that list always contains the exact same content as the original, if run outside of BCML.

The values changed are semi-random. The following picture illustrates a small excerpt of keys:

All three of these are different merges. (The one on the left has errors elsewhere)

I'm planning on a band-aid fix by only converting the mod ASDefines, so that (for these mods at least) nothing about the ASDefines ever changes.

tl;dr- No idea where the faulty code is, going to try to fix it on BCML's end.

[leoetlino]

Okay, I think I've figured out the problem. It's a lifetime issue (not in the C++ code but in the Python interop glue code). First, two simple examples:

# setup
plist = oead.aamp.ParameterList()
plist.objects[0] = oead.aamp.ParameterObject()
plist.objects[0].params["test"] = Parameter(FixedSafeString64("foo"))

# keep a reference to the param
param = plist.objects[0].params["test"]

# recreate the ParameterObject; this destructs the existing object and all its params
plist.objects[0] = oead.aamp.ParameterObject()
print(repr(param))  # error, param points to a Parameter that no longer exists

A similar issue can arise when keeping references to nested BYML containers:

d = oead.byml.Hash()
d["foo"] = oead.byml.Array([])
arr = d["foo"]
print(len(arr)) # 0
d["foo"] = oead.byml.Array([])
print(len(arr))
# garbage, because the previous value of d["foo"] has been destructed
# and d["foo"] now contains another object

This is because all objects live on the C++ side so variables like arr (in the BYML example) and param (in the AAMP one) are merely references/pointers to a C++ oead object. And when you destruct the object that is referenced by destructing or reconstructing its parent, bad things can happen (but may or may not happen - issues may not show up in a deterministic way).

My recommendation is to be careful about object lifetimes when interacting with native code because the way languages such as C++ and Rust manage memory and the Python way are wildly different, and interop glue code cannot hide every difference :)

As a rule of thumb, if you are mutating a container (e.g. hash/array/parameter list/parameter object) then don't keep references to its children as they may get invalidated. (This is similar to how you shouldn't modify a Python list/dict while iterating over it)

And now back to BCML:

    def merge_asdefine(plist: ParameterList, other_plist: ParameterList):
        # note: the type annotation is actually misleading: the values in defs are Parameters,
        # not Python strings
        defs: Dict[str, str] = {}
        for _, pobj in plist.objects.items():
            defs[str(pobj.params["Name"].v)] = pobj.params["Filename"].v
        for _, other_pobj in other_plist.objects.items():
            defs[str(other_pobj.params["Name"].v)] = other_pobj.params["Filename"].v
 
        for i, (k, v) in enumerate(defs.items()):
            key = f"ASDefine_{i}"
            if not key in plist.objects:
                plist.objects[key] = ParameterObject()
                # ^ this is what causes the issue: the parent object for parameters is destructed...
            plist.objects[key].params["Name"] = Parameter(FixedSafeString64(k))
            plist.objects[key].params["Filename"] = Parameter(v)
            # ...and then the parameter is used here even though the underlying object is dead
            # (destructed when the ParameterObject replaced the old ParameterObject)

tl;dr: there was/is a latent bug in the ASDefine merger. The recent pybind11 update merely causes the merger code to fail in a more obvious way. This is something that needs to be fixed downstream and it is not an oead bug so I am closing this.

[GingerAvalanche]

Hrm. It's not the Parameters that are getting stored, it's the FixedSafeString64s. Though I guess those would also be destructed when their parent Parameters were.

But you can see that plist.objects[key] is only assigned if key was not already in plist.objects. Since it's been verified that nothing is at that key, there shouldn't be anything getting destructed.

I guess it might be that when the FixedSafeString64 is getting assigned as the Filename parameter, its old Parameter gets destructed. The pointer in the FixedSafeString64 is kept around, but since pybind has destructed the backing string, that pointer now points to whatever happens to get put in that spot, instead. Interesting.

Thanks for the info. I'll copy the string instead of making another reference to the SafeString, and that should fix it

[leoetlino]

Ah yeah, the FixedSafeString64s are getting stored, not the Parameters. But the issue is the same as the lifetime of the string is tied to that of the Parameter.

But you can see that plist.objects[key] is only assigned if key was not already in plist.objects. Since it's been verified that nothing is at that key, there shouldn't be anything getting destructed.

I think the existence check is actually bugged; the string may need to be converted into a oead.Name first

[GingerAvalanche]

I’m pretty sure it wraps the query value in a Name if it isn’t one, or something along those lines. At the very least, I’ve used raw strings in that query syntax for years, and it’s always checked for the existence of the hash. It’s only when I’m pulling keys out that I have ever dealt with Names.

…

On Dec 26, 2022, at 2:19 AM, Léo Lam ***@***.***> wrote:  Ah yeah, the FixedSafeString64s are getting stored, not the Parameters. But the issue is the same as the lifetime of the string is tied to that of the Parameter. But you can see that plist.objects[key] is only assigned if key was not already in plist.objects. Since it's been verified that nothing is at that key, there shouldn't be anything getting destructed. I think the existence check is actually bugged; the string may need to be converted into a oead.Name first — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

[leoetlino]

Fun, looks like that is the actual change in behaviour in pybind11 that uncovered the existing lifetime issue:

# 1.2.4 (which uses pybind11 ~2.6.0)
>>> import oead
>>> l = oead.aamp.ParameterList()
>>> l.objects[1] = oead.aamp.ParameterObject()
>>> 1 in l.objects
True

The same test returns False with pybind11 >= 2.8.0 as a (unintended?) consequence of pybind/pybind11#3310, which adds an overload of __contains__ that always returns False if the type of the argument to __contains__ does not exactly match the map key type (oead.aamp.Name in our case). Unlike the lifetime thing, I would definitely consider this to be an oead issue as this behaviour isn't supposed to change. Let me fix this

[leoetlino]

Should be fixed now:

>>> import oead
>>> l = oead.aamp.ParameterList()
>>> l.objects[1] = oead.aamp.ParameterObject()
>>> l.objects.__contains__()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __contains__(): incompatible function arguments. The following argument types are supported:
    1. (self: aamp.ParameterObjectMap, arg0: object) -> bool
    2. (self: aamp.ParameterObjectMap, arg0: oead.aamp.Name) -> bool
    3. (self: aamp.ParameterObjectMap, arg0: object) -> bool

Invoked with: <oead.aamp.ParameterObjectMap object at 0x7f0a9e977470>
>>> 1 in l.objects
True
>>> 2 in l.objects
False
>>> oead.aamp.Name(1) in l.objects
True
>>> l.objects["foo"] = oead.aamp.ParameterObject()
>>> oead.aamp.Name("foo") in l.objects
True
>>> "foo" in l.objects
True

I'll make a new release later today.

[leoetlino]

v1.2.6 should be available on PyPI within half an hour. Thanks @HGStone and @GingerAvalanche for helping debug the issue :)