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

Spd/Spr Merging #10

Merged
merged 52 commits into from
Jan 12, 2024
Merged

Spd/Spr Merging #10

merged 52 commits into from
Jan 12, 2024

Conversation

Secre-C
Copy link
Contributor

@Secre-C Secre-C commented Nov 8, 2023

No description provided.

Copy link
Owner

@Sewer56 Sewer56 left a comment

Choose a reason for hiding this comment

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

Some Misc Questions

What is the Use Case for Adding New Sprites?

i.e. How are the sprites referenced. Are they referenced in scripts? (.flow, etc.)

I ask because if the user adds a sprite in order to add something that was not in the original game, and another mod also uses that same sprite index to add its own new sprite, that should ideally be resolved, so both mods can get their 'new' features.

Do we have the capabilities to resolve these conflicts down the road? I think it's possible, but I need to ensure it is.

Can this be a Stream rather than a File Emulator

We're currently parsing out the archive, and building a new archive in memory, keeping all of the new data in RAM. When textures are involved, this is not ideal; as textures are generally a large memory hog, especially since mod authors in games have a habit of throwing Super Ultra Mega 4K texture packs without consideration of how the asset is displayed on the screen.

This means they sometimes wind up making textures at mixed resolutions, 8K, 4K, 16K screen etc. Risking storing 100s of MB in RAM is not ideal.

In the case of BF Emulator, which this code seems inspired by, it wasn't very possible as data from the original file wasn't reusable; however the SPD(s) are essentially archive files, without any sort of SOLID compression, etc.

For the texture data, however, you should access the original textures, and the new textures via a FileSliceStream, such as FileSliceStreamW32; such that they are loaded from disk, and not from memory at runtime. (see: AFS Redirector and other Stream Based Redirectors)

Unit Tests Requested

Unit tests for the new Emulator should ideally be added. I'm surprised you didn't, as it's faster to test emus via unit testing rather than directly booting the game.


Separation of Concerns / 'Pay for What You Need'

When it's ported to Rust, circa late 2024, SPD and SPR Emulator should ideally be split. I have a very strong belief that end users should only pay for what they use; so code for SPR if game only uses SPD isn't very useful, and vice versa.

For now this is fine, but for R3, Persona Essentials itself will likely be split per game, and transitive dependencies like SPR/SPD emulator would only be enabled if any mod that depends on essentials uses them. i.e. a mod will be able to specify a dependency on another mod without automatically enabling that mod

docs/emulators/spd.md Show resolved Hide resolved
docs/emulators/spd.md Outdated Show resolved Hide resolved
Emulator/SPD.File.Emulator/Utilities/SpriteChecker.cs Outdated Show resolved Hide resolved
Emulator/SPD.File.Emulator/Sprite/SpriteBuilderFactory.cs Outdated Show resolved Hide resolved
Emulator/SPD.File.Emulator/SpdEmulatorApi.cs Show resolved Hide resolved
Emulator/SPD.File.Emulator/SpdEmulatorApi.cs Show resolved Hide resolved
Emulator/SPD.File.Emulator/Spd/SpdTextureEntry.cs Outdated Show resolved Hide resolved
Emulator/SPD.File.Emulator/Spd/SpdSpriteEntry.cs Outdated Show resolved Hide resolved
Emulator/SPD.File.Emulator/Spd/SpdBuilder.cs Show resolved Hide resolved
@Secre-C
Copy link
Contributor Author

Secre-C commented Nov 10, 2023

What is the Use Case for Adding New Sprites?

i.e. How are the sprites referenced. Are they referenced in scripts? (.flow, etc.)

Currently the only way sprites are referenced is in the executable, unless you're utilizing my unhardcoded toolkit mod. In terms of potential conflicts for new sprites, If two mods add the same new sprite id, one will overwrite the other, there's no way to resolve that in code that I can think of...

Can this be a Stream rather than a File Emulator

yea probably :P I'll look into converting it

Unit Tests Requested

:hee_melt: (It somehow didn't register to me that there was a test project)

I have the tests all commented out and I didn't add the spd emulator as a project reference because doing so would require updating reloaded.memory to the newest version.

The problem with this is it means every dependency needs to be updated, and the dependencies for the dependencies need to be updated, and code needs to be changed because of changed/removed methods. I spent about an hour trying to do it and gave up :/
@Secre-C
Copy link
Contributor Author

Secre-C commented Nov 14, 2023

Everything you mentioned should be addressed (kinda, read my commit on the tests), hopefully I correctly understood everything you wanted me to do 🥴

@Sewer56 Sewer56 merged commit a2e7b8e into Sewer56:main Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants