[SM64] Combined Export Panel & Behavior Script Exporting #284
Conversation
…t exporting support
…l selected objects. Edited custom path exports. Cleaned up code
…ting bhv with no bhv
There was a problem hiding this comment.
Export level SHOULD NOT be in the same panel as combined object exports, especially if the name of the panel is "SM64 Combined Object Exporter", the positioning of the level prop and the fact that this will cause confusion while exporting for both actor exports and level exports.
|
I'm unsure if I like stuff like "Export behavior" and "Export collision" to be on by default |
|
All behavior scripts should start with a BEGIN command, maybe don´t make the user add those manually and just add an enum for the object list? Also maybe adding a warning for when the behavior script doesn´t contain an END_LOOP, BREAK, DEACTIVATE, GOTO or RETURN could be a neat addition (only a warning because the user could have their own custom command which exits out of the behavior script). And if you are willing to step into feature creep territory, having toggles for common vanilla object flags would be very convenient. I don´t know what else could be done to help the user not mess up the behaviors. |
fast64_internal/sm64/sm64_objects.py
Outdated
| prop_split(col, self, "collision_object", "Collision Obj") | ||
| if self.export_gfx and not self.export_all_selected: | ||
| prop_split(col, self, "graphics_object", "Geo Layout Obj") |
There was a problem hiding this comment.
Is there a reason to abbreviate these?
Also fast64 uses "Geolayout" instead of "Geo Layout" everywhere else
| insert_line = 0 | ||
| alt_insert_line = 0 | ||
| match_line = 0 | ||
| for j, line in enumerate(file_lines): |
There was a problem hiding this comment.
Is there any reason you use j over i for loops? Typically you use j for a second index and i for the first index
There was a problem hiding this comment.
habits from electrical studies no real reason
|
|
||
| def export_group_script_load(self, script_path, props): | ||
| if not script_path.exists(): | ||
| PluginError(f"could not find {script_path.stem}") |
There was a problem hiding this comment.
A lot of your plugin errors start with lower case, but some of yours and all others in fast64 use normal casing.
| return {"CANCELLED"} | ||
|
|
||
| props.context_obj = None | ||
| # you've done it!~ |
| if props.export_all_selected: | ||
| return | ||
| else: | ||
| raise Exception(e) |
There was a problem hiding this comment.
Why not just do if not props.export_all_selected:
There was a problem hiding this comment.
Unresolving this, but also I personally do this in these kinds of exceptions as much as possible, in here it may seem less useful but having a full traceback is usually good, pylint warns for it
| if props.export_all_selected: | |
| return | |
| else: | |
| raise Exception(e) | |
| except Exception as exc: | |
| # pass on multiple export, throw on singular | |
| if not props.export_all_selected: | |
| raise Exception(exc) from exc |
Not necessarily true. Behaviors can exist as partial scripts that are jumped to as an immediate example, and also for objects in the default list the BEGIN cmd is redundant. If the user wants to set up a prototypical behavior quickly they can use a preset, such as a the solid object one.
Good ideas I will incorporate these
I am not sure this would work well while not bloating the UI and also being flexible.
I don't believe in excessively controlling user output. It should just be what they type, otherwise it may be inflexible. |
The flexibility is not an issue, but I agree the ui bloat could be, up to you I guess |
…d cleaned formatting
…rking if obj was not selected
black formatting
|
I have made the following improvements:
|
| icon="ERROR", | ||
| ) | ||
|
|
||
| info_box = box.box() |
|
yeah I'm not against renaming it. I just added context so I seemed like a
normal programmer.
I generally close the problems I won't address/already have.
…On Fri, Feb 2, 2024, 6:38 AM Lila ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In fast64_internal/sm64/sm64_objects.py
<#284 (comment)>:
> @@ -1403,6 +1420,784 @@ def draw_act(self, obj, layout, value):
layout.prop(obj, "sm64_obj_use_act" + str(value), text="")
+class AddBehavior(bpy.types.Operator):
+ bl_idname = "bone.add_behavior_script"
still, shouldn´t it be named scene.add_behavior_script instead?
—
Reply to this email directly, view it on GitHub
<#284 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABTTURW6GO4C3XP7PTZJJLDYRTF2BAVCNFSM6AAAAABBSJ2WJ6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNJZGA2DMNZYG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
…Added option to filter out object type suffixes while exporting. Made behaviors added with UI operator default to expanded. Made pathing preview more accurate.
|
The latest commit should address all the review concerns and also added extra control over object naming. |
|
The big issue with this is hackersm64, what are your plans to address enum model ids and level scripts being included in the actors folder (was reverted for the time being but is still planned afaik, fazana said grep could be an easy solution for this one) |
|
I have no plans to address a change not yet implemented in a specific repo. All features of this panel can be enabled/disabled independently, if a specific repo does not work with a feature, they can turn it off. If there is support for specific repos and versions of repos I may address this but I don't believe I have any measure currently to handle this. |
| export_col: bpy.props.BoolProperty( | ||
| name="Export Collision", | ||
| default=False, | ||
| description="Export collision for linked or selected mesh that have collision data", |
There was a problem hiding this comment.
Don´t add final punctuation in descriptions
| ) | ||
| export_col: bpy.props.BoolProperty( | ||
| name="Export Collision", | ||
| default=False, |
There was a problem hiding this comment.
Personally I'd remove all default=False as that is the default for that argument anyways, it doesn´t make anything more explicit as defaulting to false is the norm for booleans and other parts of this same pr don't specify it, to me it is very much just code bloat
| old_scene_props_to_new = { | ||
| "geoLevelName": "custom_export_name", | ||
| "geoExportPath": "custom_export_path", | ||
| "geoName": "object_name", | ||
| } |
There was a problem hiding this comment.
missing geoGroupName, doesn´t handle col but that is fine as there is rarely a case where someone would have a seperate file for just a collision export
| rootObj.select_set(True) | ||
| bpy.context.view_layer.objects.active = rootObj | ||
| if not meshGeolayout.has_data(): | ||
| raise "No gfx data to export, gfx export cancelled" |
There was a problem hiding this comment.
Raise with no exception type?
| "LOAD_MIO0_TEXTURE", | ||
| "LOAD_YAY0", | ||
| "LOAD_YAY0_TEXTURE", | ||
| "LOAD_RAW", |
There was a problem hiding this comment.
Could you add in LOAD_VANILLA_OBJECTS for hackersm64
|
|
||
| elif self.export_header_type == "Actor": | ||
| prop_split(box, self, "group_name", "Group Name") | ||
| if self.group_name == "custom": |
| # level export header | ||
| level_name: bpy.props.EnumProperty(items=enumLevelNames, name="Level", default="bob") | ||
| # actor export header | ||
| group_name: bpy.props.EnumProperty(name="Group Name", default="group0", items=groups_obj_export) |
There was a problem hiding this comment.
How about just Group for the name, since this is an enum and I heavily dislike this kind of repetition personally
| icon="ERROR", | ||
| ) | ||
|
|
||
| info_box = box.box() |
| if self.export_col and not self.export_all_selected: | ||
| prop_split(box, self, "collision_object", "Collision Obj") | ||
| if self.export_gfx and not self.export_all_selected: | ||
| prop_split(box, self, "graphics_object", "Geo Layout Obj") | ||
| if self.export_gfx and self.export_script_loads: | ||
| prop_split(box, self, "model_id", "Model ID Num") |
There was a problem hiding this comment.
You should try including these in their respective export boxes, looks really good imo
| split = layout.split(factor=0.333) | ||
| if self.export_col: | ||
| box = split.box() | ||
| box.prop(self, "export_col") | ||
| box.prop(self, "include_children") | ||
| box.prop(self, "export_rooms") | ||
| else: | ||
| split.prop(self, "export_col") | ||
| if self.export_gfx: | ||
| box = split.box() | ||
| box.prop(self, "export_gfx") | ||
| box.prop(self, "export_script_loads") | ||
| else: | ||
| split.prop(self, "export_gfx") | ||
| if not self.export_all_selected: | ||
| split.prop(self, "export_bhv") |
There was a problem hiding this comment.
Another ui suggestion, have you tried a row with align and then using toggle=1 for the export_ props? I tried it and it looks very clean imo, oot does it a lot and i love it (also using .column())
|
bump #284
|
This PR aims to streamline exporting for SM64 all into one panel. This PR is made for C exclusively, and binary features should remain unchanged.
Along with all of the above improvements to exporting. I have also added in behavior script exporting.
Behavior scripts will be exported using the same properties as above, and they also have the option to inherit the following properties from the exported object:
Behavior scripts come with several preset behaviors and support adding, deleting and editing args of all the existing original sm64 behavior script cmds.