[MOD COMPATIBILITY] Custom Tunic Models by digita-LUNA · Pull Request #3122 · HarbourMasters/Shipwright

digita-LUNA · 2023-08-15T19:42:54Z

Adds the ability to load a custom model for each tunic on Adult Link instead of using the same model for each one. The models are as follows:

objects/object_link_boy_kokiri/gLinkKokiriTunicSkel: Kokiri Tunic
objects/object_link_boy_goron/gLinkGoronTunicSkel: Goron Tunic
objects/object_link_boy_zora/gLinkZoraTunicSkel: Zora Tunic

If one of these models doesn't exist, then gLinkAdultSkel is loaded as usual.

Restrictions

Currently, tunics can't have unique face textures
Only has a new model for the body, so hands are all shared

Ship.Of.Harkinian.Directx.11.2023-08-15.15-13-44.mp4

Build Artifacts

only custom tunic models that actually exist should render now

PurpleHato

I'm genuinely pleased with what I'm observing here. While I might not be the most familiar with handling actors, it appears to be in good shape from my perspective

soh/assets/soh_assets.h

soh/soh/resource/type/Skeleton.cpp

Couchy · 2023-08-19T13:28:36Z

soh/soh/OTRGlobals.h


 uint64_t GetPerfCounter();
-struct SkeletonHeader* ResourceMgr_LoadSkeletonByName(const char* path, SkelAnime* skelAnime);
+struct SkeletonHeader* ResourceMgr_LoadSkeletonByName(const char* path, SkelAnime* skelAnime, Actor* actor);


Why is passing the Link actor necessary? Why not just check that the path is for the original Link skeleton?

#define dgLinkAdultSkel "__OTR__objects/object_link_boy/gLinkAdultSkel"

The issue appears to be that we don't have access to the path at the point later in the code where we actually need to update Skeletons (during Graph_ProcessGfxCommands). And since we're storing a pointer to the actor it's not like we're copying a bunch of extra data around so that's good. Also I could see that enabling more complex model swaps in the future, which is cool. That being said, at first glance I am a bit concerned about a use-after-free type of issue. Are we certain that the actor is guaranteed to still be loaded while the Skeleton is in that array?

Oh wait, I missed that vanillaSkeletonPath is part of SkeletonPatchInfo. So yeah just check for that. That should also simplify the checks in UpdateTunicSkeletons() since IIRC young link has a different path for his skeleton so you don't need the LINK_IS_ADULT checks or the special case skeleton load for child.

I didn't even think of that, oh my god. I have to undo all of those changes to code now, gonna go cry

So, I tried testing this, and it turns out I can't seem to compare to vanillaSkeletonPath

now that I think about this, would checking vanillaSkeletonPath work the same way as checking the actor ID would? If Link was changing form one unique tunic model to another, would it properly detect link?

leggettc18

This looks really cool, but there seems to be a way of doing it that doesn't require adding any parameters to existing function calls and structs, see comments in the code.

soh/soh/OTRGlobals.cpp

leggettc18 · 2023-08-19T19:27:36Z

soh/soh/OTRGlobals.h


 uint64_t GetPerfCounter();
-struct SkeletonHeader* ResourceMgr_LoadSkeletonByName(const char* path, SkelAnime* skelAnime);
+struct SkeletonHeader* ResourceMgr_LoadSkeletonByName(const char* path, SkelAnime* skelAnime, Actor* actor);


The issue appears to be that we don't have access to the path at the point later in the code where we actually need to update Skeletons (during Graph_ProcessGfxCommands). And since we're storing a pointer to the actor it's not like we're copying a bunch of extra data around so that's good. Also I could see that enabling more complex model swaps in the future, which is cool. That being said, at first glance I am a bit concerned about a use-after-free type of issue. Are we certain that the actor is guaranteed to still be loaded while the Skeleton is in that array?

leggettc18 · 2023-08-19T19:34:56Z

soh/soh/OTRGlobals.h


 uint64_t GetPerfCounter();
-struct SkeletonHeader* ResourceMgr_LoadSkeletonByName(const char* path, SkelAnime* skelAnime);
+struct SkeletonHeader* ResourceMgr_LoadSkeletonByName(const char* path, SkelAnime* skelAnime, Actor* actor);


Oh wait, I missed that vanillaSkeletonPath is part of SkeletonPatchInfo. So yeah just check for that. That should also simplify the checks in UpdateTunicSkeletons() since IIRC young link has a different path for his skeleton so you don't need the LINK_IS_ADULT checks or the special case skeleton load for child.

soh/soh/resource/type/Skeleton.cpp

…cluded in code

soh/src/overlays/actors/ovl_player_actor/z_player.c

inspectredc · 2023-09-23T17:27:48Z

So I have tried setting this up but keep running into some problems which I'm posting here with the hope that I can further understand the process of how this is intending to work as well as give a few code reviewing notes.

I created a modded otr for the goron tunic skeleton which was just a copy of the original gLinkAdultSkel imported in blender and exported using the new for this tunic mod. I then also included a mod for gLinkAdultSkel itself so that I would be able to tell the difference on equipping the goron tunic. When I first equipped the goron tunic with Alt Assets turned on, then no change in model/skeleton occured (the new skeleton loaded returns nullptr), and on equipping with Alt Assets turned off my game would crash. I'm not sure if the crash is a result of me using a bad otr or if I'm doing some part of the process wrong so would appreciate any help with that. As for when alternate assets is turned on, this seems like unintentional behaviour since you would want a non-alt appended asset to load regardless of the CVar. Perhaps my understanding of what is going on is wrong and this is also a result of the otr being wrong though.

Regarding the code, I agree with the other reviewers that it would be for the best to do a check with the vanillaSkeletonPath, specifically checking if it is equal to objects/object_link_boy/gLinkAdultSkel. I see you mentioned issues with checking against it so would love to know more detail about what the problem you encountered was.

I'm not sure how others feel about the best way to approach adding sections to the source code, but I think Couchy made a fair point about clearly separating the code. While other changes haven't always made sure this is the case, I think it's always appreciated if you make it clear what is being introduced and for what reason. You could also still put it behind a CVar as suggested and default to the CVar being set to true, with the cvar name serving as the explanation reason, and also CVars do not always necessarily need to be exposed in the enhancements menu anyway to address your concerns.

inspectredc · 2023-09-28T13:32:44Z

soh/soh/resource/type/Skeleton.cpp

+        if (skel.actor != nullptr) // is there actually an actor to check?
+        {
+            if (skel.actor->id == 0) // is this Link?
+            {
+                Skeleton* newSkel;
+                if (LINK_IS_ADULT && CUR_EQUIP_VALUE(EQUIP_TUNIC) - 1 == PLAYER_TUNIC_KOKIRI &&
+                    ResourceGetIsCustomByName(gLinkKokiriTunicSkel)) {
+                    newSkel = (Skeleton*)LUS::Context::GetInstance()
+                                  ->GetResourceManager()
+                                  ->LoadResource((isAlt ? LUS::IResource::gAltAssetPrefix : "") + gLinkKokiriTunicSkel, true)
+                                  .get();
+                } else if (LINK_IS_ADULT && CUR_EQUIP_VALUE(EQUIP_TUNIC) - 1 == PLAYER_TUNIC_GORON &&
+                           ResourceGetIsCustomByName(gLinkGoronTunicSkel)) {
+                    newSkel = (Skeleton*)LUS::Context::GetInstance()
+                                  ->GetResourceManager()
+                                  ->LoadResource((isAlt ? LUS::IResource::gAltAssetPrefix : "") + gLinkGoronTunicSkel, true)
+                                  .get();
+                } else if (LINK_IS_ADULT && CUR_EQUIP_VALUE(EQUIP_TUNIC) - 1 == PLAYER_TUNIC_ZORA &&
+                           ResourceGetIsCustomByName(gLinkZoraTunicSkel)) {
+                    newSkel = (Skeleton*)LUS::Context::GetInstance()
+                                  ->GetResourceManager()
+                                  ->LoadResource((isAlt ? LUS::IResource::gAltAssetPrefix : "") + gLinkZoraTunicSkel, true)
+                                  .get();
+                } else { // child link, no model available
+                    newSkel = (Skeleton*)LUS::Context::GetInstance()
+                                  ->GetResourceManager()
+                                  ->LoadResource(
+                                      (isAlt ? LUS::IResource::gAltAssetPrefix : "") + skel.vanillaSkeletonPath, true)
+                                  .get();
+                }
+
+                if (newSkel != nullptr)
+                    skel.skelAnime->skeleton = newSkel->skeletonData.skeletonHeader.segment;
+            }
+        }


Suggested change

if (skel.actor != nullptr) // is there actually an actor to check?

{

if (skel.actor->id == 0) // is this Link?

{

Skeleton* newSkel;

if (LINK_IS_ADULT && CUR_EQUIP_VALUE(EQUIP_TUNIC) - 1 == PLAYER_TUNIC_KOKIRI &&

ResourceGetIsCustomByName(gLinkKokiriTunicSkel)) {

newSkel = (Skeleton*)LUS::Context::GetInstance()

->GetResourceManager()

->LoadResource((isAlt ? LUS::IResource::gAltAssetPrefix : "") + gLinkKokiriTunicSkel, true)

.get();

} else if (LINK_IS_ADULT && CUR_EQUIP_VALUE(EQUIP_TUNIC) - 1 == PLAYER_TUNIC_GORON &&

ResourceGetIsCustomByName(gLinkGoronTunicSkel)) {

newSkel = (Skeleton*)LUS::Context::GetInstance()

->GetResourceManager()

->LoadResource((isAlt ? LUS::IResource::gAltAssetPrefix : "") + gLinkGoronTunicSkel, true)

.get();

} else if (LINK_IS_ADULT && CUR_EQUIP_VALUE(EQUIP_TUNIC) - 1 == PLAYER_TUNIC_ZORA &&

ResourceGetIsCustomByName(gLinkZoraTunicSkel)) {

newSkel = (Skeleton*)LUS::Context::GetInstance()

->GetResourceManager()

->LoadResource((isAlt ? LUS::IResource::gAltAssetPrefix : "") + gLinkZoraTunicSkel, true)

.get();

} else { // child link, no model available

newSkel = (Skeleton*)LUS::Context::GetInstance()

->GetResourceManager()

->LoadResource(

(isAlt ? LUS::IResource::gAltAssetPrefix : "") + skel.vanillaSkeletonPath, true)

.get();

}

if (newSkel != nullptr)

skel.skelAnime->skeleton = newSkel->skeletonData.skeletonHeader.segment;

}

}

if (strcmp(skel.vanillaSkeletonPath.c_str(), &gLinkAdultSkel[OTR_OFFSET]) == 0) {

Skeleton* newSkel = nullptr;

if (CUR_EQUIP_VALUE(EQUIP_TUNIC) - 1 == PLAYER_TUNIC_KOKIRI) {

newSkel = (Skeleton*)LUS::Context::GetInstance()

->GetResourceManager()

->LoadResource((isAlt ? LUS::IResource::gAltAssetPrefix : "") + gLinkKokiriTunicSkel, true)

.get();

} else if (CUR_EQUIP_VALUE(EQUIP_TUNIC) - 1 == PLAYER_TUNIC_GORON) {

newSkel = (Skeleton*)LUS::Context::GetInstance()

->GetResourceManager()

->LoadResource((isAlt ? LUS::IResource::gAltAssetPrefix : "") + gLinkGoronTunicSkel, true)

.get();

} else if (CUR_EQUIP_VALUE(EQUIP_TUNIC) - 1 == PLAYER_TUNIC_ZORA) {

newSkel = (Skeleton*)LUS::Context::GetInstance()

->GetResourceManager()

->LoadResource((isAlt ? LUS::IResource::gAltAssetPrefix : "") + gLinkZoraTunicSkel, true)

.get();

}

if (newSkel != nullptr) {

skel.skelAnime->skeleton = newSkel->skeletonData.skeletonHeader.segment;

}

}

This should be a way to remove the need for passing the actor around (and no adult link checks needed either!) the OTR_OFFSET will need to be defined in this case as 7 (the length of the string 'OTR')

Going to look into the alt toggling issue now

Also let me know if you want me to PR the changes to your branch or help in any other way with these changes!

inspectredc · 2023-09-28T15:13:22Z

I think i understand the alt toggling issues now too. The code for updating skeletons works on the premise that the skeletons have already been registered and therefore exist when the alt skeleton path returns null. This means for the tunics, if the alt asset toggle is turned on, then both the non-alt skeleton path and alt skeleton path will need to be checked, with the alt skeleton overriding the non-alt skeleton if it exists.

I'll PR what that would look like to your branch this evening so you can see what the changes look like. The only other thing I'd want to get done now is to figure out a guide for the process of creating custom tunic skeleton otrs and how to avoid them causing crashes.

digita-LUNA · 2023-10-20T18:06:15Z

Closed for #3306

digita-LUNA added 5 commits August 15, 2023 14:30

Added placeholder models for each tunic

ed4c6f7

Main Code Implementation

728b053

removed placeholder models from soh.otr

fd1c8a7

only custom tunic models that actually exist should render now

why was windows letting me build

5626a13

moved BufferTunicsNextFrame to a more fitting location

5b8a104

PurpleHato reviewed Aug 16, 2023

View reviewed changes

soh/assets/soh_assets.h Outdated Show resolved Hide resolved

soh/soh/resource/type/Skeleton.cpp Outdated Show resolved Hide resolved

Couchy reviewed Aug 19, 2023

View reviewed changes

leggettc18 requested changes Aug 19, 2023

View reviewed changes

Couchy reviewed Aug 19, 2023

View reviewed changes

soh/soh/resource/type/Skeleton.cpp Outdated Show resolved Hide resolved

digita-LUNA added 3 commits August 19, 2023 23:03

Updated code used to load skeletons so that full file paths aren't in…

b41fad6

…cluded in code

Fixed Auto-formatting

2a33cbd

Re-added adult checks (for now)

d89e767

Couchy reviewed Aug 20, 2023

View reviewed changes

soh/src/overlays/actors/ovl_player_actor/z_player.c Outdated Show resolved Hide resolved

Rename Tunic assets to be easier to understand

8bbc3f2

digita-LUNA requested a review from leggettc18 August 21, 2023 23:47

Updated how tunics are toggled

45e5c43

inspectredc reviewed Sep 28, 2023

View reviewed changes

inspectredc mentioned this pull request Sep 28, 2023

Remove actor passing and fix alt skel issue digita-LUNA/Shipwright#2

Closed

inspectredc mentioned this pull request Oct 19, 2023

Custom Tunic Models #3306

Closed

digita-LUNA closed this Oct 20, 2023

Conversation

digita-LUNA commented Aug 15, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Restrictions

Build Artifacts

Uh oh!

PurpleHato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leggettc18 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

inspectredc commented Sep 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inspectredc commented Sep 28, 2023

Uh oh!

digita-LUNA commented Oct 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

digita-LUNA commented Aug 15, 2023 •

edited by github-actions bot

Loading