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

Support RomFS mode for SR #141

Merged
merged 1 commit into from
May 22, 2024
Merged

Support RomFS mode for SR #141

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/mercury_engine_data_structures/file_tree_editor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,11 @@
if output_format == OutputFormat.ROMFS:
# Clear modified_pkgs, so we don't read/write any new pkg
# We keep system.pkg because .bmmaps don't read properly with exlaunch and it's only 4MB
modified_pkgs = list(filter(lambda pkg: pkg == "packs/system/system.pkg", modified_pkgs))
# We keep MSR's "_discardables.pkg" as the files are not read from RomFS without the pkg
modified_pkgs = list(

Check warning on line 330 in src/mercury_engine_data_structures/file_tree_editor.py

View check run for this annotation

Codecov / codecov/patch

src/mercury_engine_data_structures/file_tree_editor.py#L330 

Added line #L330 was not covered by tests
filter(lambda pkg: pkg == "packs/system/system.pkg" or pkg.endswith("discardables.pkg"),
modified_pkgs)
)

# Ensure all pkgs we'll modify is in memory already.
# We'll need to read these files anyway to modify, so do it early to speedup
Expand Down Expand Up @@ -355,7 +359,11 @@
_write_to_path(output_path.joinpath(path), data)
if self._files_for_asset_id[asset_id] - {None}:
replacements.append(path)
if output_format == OutputFormat.ROMFS and asset_id in asset_ids_to_copy:
if (

Check warning on line 362 in src/mercury_engine_data_structures/file_tree_editor.py

View check run for this annotation

Codecov / codecov/patch

src/mercury_engine_data_structures/file_tree_editor.py#L362 

Added line #L362 was not covered by tests
output_format == OutputFormat.ROMFS and
asset_id in asset_ids_to_copy and
self.target_game != Game.SAMUS_RETURNS
):
del asset_ids_to_copy[asset_id]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was there a specific purpose for this, I don't see?
Just to clarifiy why I had to remove it so far:
The issue here with SR is that some asset are in asset_ids_to_copy because they are ensured (see L338-L341).
Deleting some of them doesn't work because we need to keep _disardables.pkg files, that would throw a key error in L396 if they are removed.
So, the use case is: Ensured assets which needs to be written to the discardables.pkg

Copy link
Member

Choose a reason for hiding this comment

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

ugh there was a reason. Struggling to remember why though.

What comes to mind is the need to keep things in system.pkg even if output being ROMFS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What comes to mind is the need to keep things in system.pkg even if output being ROMFS?

Hmmm isn't that the same case I have in SR with keeping _discardables.pkg? I think, the current upstream code should also throw a key error if you ensure something into the system.pkg for Dread because the lines in question here, would delete the asset from the asset_ids_to_copy.

I only see a memory save thing here? If you ensure something into a pkg but it's already written to the RomFS by this code, you don't need to write it to the pkg later again. (Of course that's only true for non system.pkg in dread).

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 think, the current upstream code should also throw a key error if you ensure something into the system.pkg for Dread because the lines in question here, would delete the asset from the asset_ids_to_copy.

Isn't true. That is really some piece of code :D.
The loop around it is only executed for _modified_resources and the if around the del checks if it is in any pkg.

E.g. to get a KeyError: 15223854729795290759 in dread with current upstream MEDS, you can take a resource from a pkg, modify it and then ensure it into one of the files which still have to be written as a pkg (so in Dread only system.pkg)

    bla = editor.get_file("actors/characters/iceflea/charclasses/iceflea.bmsad")
    bla.raw["name"] = "abc"
    editor.ensure_present("packs/system/system.pkg", "actors/characters/iceflea/charclasses/iceflea.bmsad")

Leads to a KeyError because the del is executed.

Though I'm not brain enough today to get the reason for the delete.

Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't you just slap a and if game ~= Game.SAMUS_RETURNS instead of removing it, since it sounds like Dread does use it? I could be mistaken though and not following along.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the safety method, yeah. Guess I use it then to may not break dread by accident even I think I understood what this is doing.

The whole thing could benefit from a refactor which would be like: While iterating through modified ressources, you only want to delete from asset_ids_to_copy if the asset_id is not ensured in a pkg from modified_pkgs.

Changed the commit and also will update the PR title to be correct

else:
self._toc.remove_file(asset_id)
Expand Down