-
Notifications
You must be signed in to change notification settings - Fork 100
Description
TL;DR "ADD" and "ADD_ALL" operations for SM64_AnimTableOps assume the in_table status of the header variants being operated on has already been checked and established, but the value of in_table is always assumed False in the current setup possibly as a relic of the animation inspector rewrite back in August of 2025.
SM64 animation tables are clearly intended to not have repeat header variants added to them, however as of writing this, (commit 67552ca), that is not properly implemented. I suspect this slipped through as a part of commit 8853a48 where the animation inspector was completely reworked.
I have been trying for hours to create a branch in a fork for what I thought would be a simple fix and upgrading my work environment to give me at least an IDE to work with over my previously reported issue. My issue this time isn't so much a lack of understanding what could work in this instance but rather what should work for the future of the repo. Design choices were established that would not have enough data to work with if they were continued to be followed into a PR that fixes the issue.
To start, I figured I'd just alter SM64_AnimTableOps::execute_operator() to detect the existence or lack thereof of a header variant in the table already via a query/iteration of get_anim_props(context).elements. Here is the relevant snipper of the aforementioned method as it stands as I write this:
elif self.op_name == "ADD":
if self.index != -1:
table_element = table_elements[self.index]
table_elements.add()
if self.action_name: # set based on action variant
table_elements[-1].set_variant(bpy.data.actions[self.action_name], self.header_variant)
elif self.index != -1: # copy from table
copyPropertyGroup(table_element, table_elements[-1])
if self.index != -1:
table_elements.move(len(table_elements) - 1, self.index + 1)
elif self.op_name == "ADD_ALL":
action = bpy.data.actions[self.action_name]
for header_variant in range(len(get_action_props(action).headers)):
table_elements.add()
table_elements[-1].set_variant(action, header_variant)
That implementation described above alone I don't think would be difficult. I'd just do some Googling as to the syntax for interacting with a CollectionProperty and it'd solve itself from there,. My concern is in what I suspect to be lingering design choices from before the animation inspector rewrite:
action_props.draw_props(
layout=col,
action=action,
specific_variant=self.variant,
in_table=True,
updates_table=updates_table,
export_seperately=export_seperately,
export_type=export_type,
actor_name=actor_name,
gen_enums=gen_enums,
dma=dma,
)
get_action_props(action).draw_props(
layout=col,
action=action,
specific_variant=None,
in_table=False,
updates_table=get_anim_props(context).update_table,
export_seperately=export_seperately,
export_type=sm64_props.export_type,
actor_name=get_anim_actor_name(context),
gen_enums=get_anim_props(context).gen_enums,
dma=dma_structure_context(context),
)
The above two snippets, in SM64_AnimTableElementProperties and AnimationPanelAction respectively, are the only two places across the repo where information is piped through to a header_args Object as to if a header variant is or isn't already in the table, obviously passed in via in_table to a call further in the stack. At this point in the code, under the current design, these method signatures do not have enough information regarding the context of what the user plans to do in order to be able to fill in a value for is_table, I suspect this was once the case but not anymore.
I cannot definitively prove the following, but debugging showed me that only the latter of these two snippets actually executed, the former was ignored. This is seemingly a trend with SM64_AnimTableElementProperties where it seems to remain in its entirety in the source but everything surrounding it suggests to me that it is completely unused or ignored. Not to mention in_table being either the True or False literal with no in-between.
Nevertheless, I don't want to go altering method signatures unless I am certain that in_table is no more than a relic in all instances of the method being called. Not to mention, the placement of in_table suggests to me that the detection of a header variant being in a table or not is designed to be pre-computed and passed down the call stack into the part that renders out the UI. There's already logic for hiding 'Add' buttons when in_table is True but right now that is never the case, one can add the same header variant over and over.