-
Notifications
You must be signed in to change notification settings - Fork 8
More detections for the Object Analyzer #54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
…gle-limb skeletons not existing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool
This doesn't change or add anything rendering-wise, right? Or does it allow rendering skeletons using skin/lod limbs?
case Z64Object.EntryType.StandardLimb: | ||
case Z64Object.EntryType.LODLimb: | ||
case Z64Object.EntryType.SkinLimb: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to make SkeletonStandardLimbHolder
, SkeletonLODLimbHolder
, SkeletonSkinLimbHolder
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made them the same Holder in the hopes that it will make adding the other limb types to the renderer easier. I don't think separate holders are any more beneficial in themselves either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well my complaint is more about SkeletonLimbHolder I guess ( https://github.com/Thar0/Z64Utils/blob/more-detections/Z64Utils/Z64/Z64Object.cs#L232 )
// Standard and LOD Limb Only
public SegmentedAddress DListSeg { get; set; }
// LOD Limb Only
public SegmentedAddress DListFarSeg { get; set; }
// Skin Limb Only
public int SegmentType { get; set; } // indicates the type of data pointed to by SkinSeg
public SegmentedAddress SkinSeg { get; set; }
this is kind of bad when working in an OOP context
public short Dist; | ||
} | ||
|
||
public CollisionPoly[] CollisionPolys { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public CollisionPoly[] CollisionPolys { get; set; } | |
public Poly[] polygons { get; set; } |
being in a CollisionPolygonsHolder
is probably enough to understand it's collision-related
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually when I resurrect my Collision Renderer progress it will be used outside of the Holder.
colHeader.VerticesHolder = obj.AddCollisionVertices(nVertices, off: (int)verticesSeg.SegmentOff); | ||
colHeader.PolygonsHolder = obj.AddCollisionPolygons(nPolygons, off: (int)polygonsSeg.SegmentOff); | ||
colHeader.SurfaceTypesHolder = obj.AddCollisionSurfaceTypes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it be better if the "holder linking" happened in Z64Object
I assume this being in FindCollision means importing json (or in the future zapd xml) means VerticesHolder wouldn't be set for imported collision header holders
I have a somewhat similar issue with CI textures referencing the palette texture holder
Maybe a Z64Object#updateHolderReferences
function or something could be added and called after setData
or for example AddCollisionVertices
could check in all collision header holders in the Z64Object if the holder should be used by one of them, based on the segmented address they have
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this would be more appropriate since as you say the current way doesn't work for importing. This is a more general issue that applies to other holders already in the repo though, so I'd like to hold off on this and fix them in another PR, which I am happy to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What other holders? I only know about specifically CI textures palettes (we may want to open an issue to discuss implementing the "update holder references" thing and list relevant holders)
That's correct it doesn't add any new rendering features, it just makes the data available for possible renderers to be added. |
This adds more detections to the object analyzer:
Addresses #30 and #31.
This also removes the assumption that single limb skeletons do not exist, as it has been shown they do. I'm unsure whether there are any regressions caused by this change, since as I mentioned in #48 there isn't a great way to check this currently except just trying a few objects.
Still to do: