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.

Sign up for GitHub

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

Jump to bottom

[OoT] Fix latest decomp issues #366

Merged
merged 16 commits into from
Aug 20, 2024
Merged

[OoT] Fix latest decomp issues #366

merged 16 commits into from
Aug 20, 2024

Conversation

Yanis42
Copy link
Contributor

@Yanis42 Yanis42 commented Jun 24, 2024 •
edited
Loading

Currently importing mesh doesn't work and I didn't look into it yet

@Yanis42 Yanis42 added this to the v2.2.2 milestone Jun 24, 2024
@Yanis42 Yanis42 marked this pull request as ready for review June 24, 2024 20:58
@Yanis42
Copy link
Contributor Author

Yanis42 commented Jun 24, 2024

(ready to review)

Dragorn421
Dragorn421 reviewed Jun 29, 2024
View reviewed changes
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.

These changes are not backwards compatible with repos based on decomps before assets moved to extracted/ , right?
As in, if one's mod is based on a decomp revision where assets are in assets/, with these changes fast64 would no longer work in that mod's repo?

@Yanis42
Copy link
Contributor Author

Yanis42 commented Jun 29, 2024

well, yes, I thought of that and I figured people should use something up-to-date because of the better documentation and new features in the case of HackerOoT (and actually both since we pull decomp often)

I guess it shouldn't be that hard to make it backward compatible as the extracted path comes from a function :peepoShrug:

@Yanis42 Yanis42 mentioned this pull request Jul 18, 2024
Merged
sauraen
sauraen approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@sauraen sauraen left a comment

Choose a reason for hiding this comment

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

Some nits, otherwise looks okay. Also I hope you've tested this well.

fast64_internal/oot/oot_level_parser.py Outdated Show resolved Hide resolved
fast64_internal/oot/scene/operators.py Outdated
Comment on lines 166 to 168
subfolder = (
context.scene.fast64.oot.get_extracted_path() + "assets/scenes/" + settings.subFolder + "/"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to the f-string approach shown in the next change below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a bit weird to use f-strings with the function as the value returned ends with /

though it's fine for now, I think we should use pathlib more in general anyway so it can be addressed in a general cleanup (though I'll f-string the other parts)

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 fixed the slash oddities but I didn't use f-strings eveywhere because of the line length limitation of the formatter

fast64_internal/oot/scene/panels.py Outdated
Comment on lines 85 to 87
context.scene.ootDecompPath
+ context.scene.fast64.oot.get_extracted_path()
+ f"assets/scenes/{removeSettings.subFolder}/{removeSettings.name}/"
Copy link
Contributor

Choose a reason for hiding this comment

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

More adds + f-strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one doesn't fit on one line

@Yanis42
Copy link
Contributor Author

Yanis42 commented Jul 19, 2024

I tested again with HackerOoT (where I can experiment easier between before and after), everything works for me

@Yanis42 Yanis42 mentioned this pull request Jul 19, 2024
Closed
@Yanis42
Copy link
Contributor Author

Yanis42 commented Jul 23, 2024

decomp has a PR that lets you use the older asset system for mods, idk if this version change is really necessary now, I need to think about it

@Dragorn421
Copy link
Contributor

Dragorn421 commented Aug 8, 2024 •
edited
Loading

Checklist:

  • nothing must write to or delete from extracted/, only from/to assets/ and spec
    • export to assets/ like previously
    • to delete assets, just remove from spec and don't touch extracted/
    • to replace assets, just export to assets/ . A file in assets/ has priority over a file with the same path under extracted/version/
    • to append data to an assets file, like for example to gkeep and link_animetion with exporting link animations, create files in assets/ and include the corresponding .o in the spec (also keeping the existing includes, that will use the extracted/version/ assets)
  • implement importing from extracted/
    • there must be a choice of the version to import from (import from extracted/version/)
    • there must be an option to use "legacy" behavior of importing from assets/, for people using "older" decomp

@Dragorn421 Dragorn421 mentioned this pull request Aug 11, 2024
Merged
Dragorn421
Dragorn421 reviewed Aug 15, 2024
View reviewed changes
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.

Works: 👏

  • importing skeletons
  • importing Link (child/adult)
  • importing scenes (with or without cutscenes)
  • exporting scenes
  • importing non-link animations
  • importing link animations: probably works but this branch lacks the import link anims fix (merge main)

Doesn't work: 📉

  • exporting skeletons
  • exporting Link (child/adult)

@Dragorn421 Dragorn421 added oot Has to do with the Ocarina of Time 64 side codebase Code maintenance/cleanup labels Aug 19, 2024
@Dragorn421
Copy link
Contributor

I pushed a workaround for allowing exporting skeletons/Link

I call it a workaround and not a fix because I'm not super sure the behavior is ideal, but at least it can work

Dragorn421
Dragorn421 approved these changes Aug 19, 2024
View reviewed changes
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 don't think this PR addresses the numerous problems posed by the evolving assets system in the best way, but at least it works enough to be usable. Better than nothing!

@Dragorn421 Dragorn421 added the merge soon Will be merged in a few days at most if nothing else comes up label Aug 19, 2024
krm01
krm01 approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@krm01 krm01 left a comment

Choose a reason for hiding this comment

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

glanced briefly at the code but did not review it with any scrutiny, however tested this locally and everything i tested seemed to work.

@Dragorn421 Dragorn421 merged commit 86629b0 into Fast-64:main Aug 20, 2024
1 check passed
@Dragorn421 Dragorn421 mentioned this pull request Aug 20, 2024
Merged
Lilaa3 added a commit to Lilaa3/fast64 that referenced this pull request Aug 30, 2024
@Lilaa3
[F3D/MK64] Fix Fast-64#366 regression
aeeaf88
Dragorn421 pushed a commit that referenced this pull request Sep 4, 2024
@Lilaa3
[F3D/MK64] Fix #366 regression (#440)
2fb6e31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dragorn421 Dragorn421 Dragorn421 approved these changes

@krm01 krm01 krm01 approved these changes

@sauraen sauraen sauraen approved these changes

Assignees
No one assigned
Labels
codebase Code maintenance/cleanup merge soon Will be merged in a few days at most if nothing else comes up oot Has to do with the Ocarina of Time 64 side
Projects
None yet
Milestone
v2.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Yanis42 @Dragorn421 @krm01 @sauraen