Skip to content
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

[OOT] Fix wrong segments on import #275

Closed
wants to merge 4 commits into from

Conversation

Yanis002
Copy link
Contributor

@Yanis002 Yanis002 commented Jan 5, 2024

Thanks to kenton and cdi-fails for figuring this out

@Yanis002 Yanis002 added the oot Has to do with the Ocarina of Time 64 side label Jan 5, 2024
@Yanis002 Yanis002 requested a review from Dragorn421 January 5, 2024 18:45
Comment on lines 390 to 399
if name == "gEmptyDL":
return None
return name
else:
for attr_name in dir(self.materialContext.ootMaterial.opaque):
if re.match("segment[89ABCD]", attr_name):
setattr(self.materialContext.ootMaterial.opaque, attr_name, False)
for attr_name in dir(self.materialContext.ootMaterial.transparent):
if re.match("segment[89ABCD]", attr_name):
setattr(self.materialContext.ootMaterial.transparent, attr_name, False)
return name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would like an explanation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit, I didn't look at it 4Head

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the loop is different tho, should I zip it?

@Yanis002 Yanis002 mentioned this pull request Jan 5, 2024
Copy link
Contributor

@Dragorn421 Dragorn421 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure this "fix" breaks this comment

# Don't clear ootMaterial, some skeletons (Link) require dynamic material calls to be preserved between limbs

dynamic material calls aka self.materialContext.ootMaterial is exactly what we want to clear under certain conditions (this PR implementing "clear every dlist jump", which is pretty rough imo) but this comment says they shouldn't be cleared ever for skeletons

@Yanis002 Yanis002 added the bug Something isn't working label Jan 10, 2024
@ariahiro64
Copy link

last commit broke importing entirely

@Yanis002
Copy link
Contributor Author

I tried and works on my machine™

@Yanis002 Yanis002 added this to the v2.2.1 milestone May 19, 2024
Copy link
Contributor

@Dragorn421 Dragorn421 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOTF3DContext.processDLName(name) has the following purpose to hook into the handling of gsSPDisplayList({name}):
if name is a raw segment offset like 0x08000000, don't try to find a dlist named "0x08000000" and instead mark the material as calling segment 8 (with materialContext.ootMaterial.{opaque,transparent}.segment8 = True)

(btw I don't know what is the issue this PR fixes)
The suggested fix here would mean a dlist like

gsSPDisplayList(0x08000000),
gsSPDisplayList(gSomeSymbolDL),
(vertices & triangles),

would be imported with the resulting material not calling segment 8, which sounds undesirable

Note that this:

gsSPDisplayList(gSomeSymbolDL),
gsSPDisplayList(0x08000000),
(vertices & triangles),

would have the imported material be set to call segment 8

@Yanis002
Copy link
Contributor Author

closing this for now, it seems this fix needs more testing/better implementation (also I don't really understand how materials works so I won't be able to be efficient on this one anyway)

@Yanis002 Yanis002 closed this May 20, 2024
@Dragorn421
Copy link
Contributor

This was attempting to fix #252 btw
Bit more context on fast64 discord: https://discord.com/channels/874816709855440926/874832163483287612/1192887072751616120

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working oot Has to do with the Ocarina of Time 64 side
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants