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

[Request For Comment] The Great Sorting #89

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

EliteMasterEric
Copy link
Member

This PR represents part of the changes I'm making in an internal fork. As part of the changes I am making to the game's performance, I am reworking the game's asset system to no longer use the old library system (this fixes several issues including FunkinCrew/Funkin#2615). Since this would move a bunch of stuff around, I decided this would be a good time to move everything else around as well, but since this is such a major change that will affect every modder, I wanted to have time to get feedback while nothing is set in stone yet.

  • Files are now fully placed based on focus area rather than sorting by file type. This means that, for example, both the sounds and the graphics for the main menu go in the same folder.
  • Relevant stuff for gameplay is now placed together. So for stages, the graphics, sounds, data file, and script file all go in the assets/gameplay/stages/<stageId> folder. Same for stuff like characters and charts (the instrumental finally goes in the same folder as the chart file!)
  • I renamed a bunch of stuff, trying to stick with lower-kebab-case for file names where appropriate, with some exceptions (for example, renaming Inst.ogg would break older chart files so I decided not to do that).
  • This change isn't necessarily coming in v0.6, it'll come whenever it's done.
  • This WILL be a major breaking change for mods and most mods will have to rearrange their stuff and fix their scripts because they WILL be broken after the update. This is something I want to get done sooner rather than later so we can rip the band-aid off, so to speak.

If you have any improvements to maximize the benefits of this change, or if you don't like something about this rework, speak up! The more constructive, the better.

@Keoiki
Copy link

Keoiki commented Oct 25, 2024

It all looks fine to me on first glance. I didn't like that the chart and audio files were in different places since .fnfc effectively had them in the same place.

What are the changes to Assets/Paths classes, it seems some functions have shifted from Paths to Assets, new ones added(?) and additional functions are called directly after Paths functions (such as toString, toFlxSoundAsset and toFlxGraphicAsset). What's up with those?

@CyndaReal
Copy link

How exactly will this improve performance? If it's only marginal, I don't see how this improves much of anything. I do think the assets need restructuring, particularly ditching the week based library system and separating former week libraries into stage libraries, but this feels like it only makes the structuring more confusing.

@AbnormalPoof
Copy link
Collaborator

AbnormalPoof commented Oct 25, 2024

  1. How would caching/preloading work with this new system?
  2. Will there eventually be a PR for the code side on the main repository so we can test the new asset system?

@EliteMasterEric
Copy link
Member Author

The code side of things is still a work in progress, but the basic idea is that Paths.image now returns an AssetPath object instead of a string, and functions which take an asset path now take an AssetPath instead of a string, in order to guarantee that people don't just pass random strings without the game validating they are valid paths.

As for caching, when switching states the game will request a list of all the assets that a given state needs to work, and cache those (using multi-threading!) before unloading the assets that are no longer needed and finally switching to that state. This is what the queryAssets functions in some of the scripts are for.

@AbnormalPoof
Copy link
Collaborator

AbnormalPoof commented Oct 25, 2024

Interesting.

Of note, I do feel like song audio should be separated into folders with their variation names, since it could get somewhat messy if a lot of variations for a specific song exist.

Maybe something like this?:

|-ugh
  |-audio
    |-default
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
    |-erect
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
    |-pico
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-pico.ogg
    |-dsides
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
    |-bsides
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg

Alternatively, if song audio and chart data need to be next to each other:

|-ugh
  |-variations
    |-default
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
      |-metadata.json
      |-chart.json
    |-erect
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
      |-metadata.json
      |-chart.json
    |-pico
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-pico.ogg
      |-metadata.json
      |-chart.json
    |-dsides
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
      |-metadata.json
      |-chart.json
    |-bsides
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
      |-metadata.json
      |-chart.json

@CyndaReal
Copy link

Interesting.

Of note, I do feel like song audio should be separated into folders with their variation names, since it could get somewhat messy if a lot of variations for a specific song exist.

Maybe something like this?:

|-ugh
  |-audio
    |-default
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
    |-erect
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
    |-pico
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-pico.ogg
    |-dsides
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
    |-bsides
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg

Alternatively, if song audio and chart data need to be next to each other:

|-ugh
  |-variations
    |-default
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
      |-metadata.json
      |-chart.json
    |-erect
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
      |-metadata.json
      |-chart.json
    |-pico
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-pico.ogg
      |-metadata.json
      |-chart.json
    |-dsides
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
      |-metadata.json
      |-chart.json
    |-bsides
      |-Inst.ogg
      |-Voices-tankman.ogg
      |-Voices-bf.ogg
      |-metadata.json
      |-chart.json

THIS! I've felt this was cluttery since the pico update dropped!

@doggogit
Copy link
Contributor

doggogit commented Oct 26, 2024

I've opened #90 to address some of my thoughts about this rework, this is definitely a step in the right direction, but I've also added my own flavor to this wonderful basket of goodies

Please do feel free to talk about potential changes and thoughts, anything is welcome

@Burgerballs
Copy link

How will this affect custom states?

@KoloInDaCrib
Copy link

KoloInDaCrib commented Oct 28, 2024

Honestly I think this change would be great! My only real complaint besides AbnormalPoof's requested changes is that I am not too keen on having some chart editor dialogs be accessible easily through the ui folder. I am aware this was done in order for RuntimeComponentBuilder to work but making them public is just going to encourage tampering with xml dialog files, which will either have little to no effect on programmed functionality or just lead to a bunch of crashes due to missing components/invalid xml structure. This has bothered me since the launch of 0.3

also rest in peace directory field from stage datas you were made obsolete

@EliteMasterEric
Copy link
Member Author

EliteMasterEric commented Oct 28, 2024

Honestly I think this change would be great! My only real complaint besides AbnormalPoof's requested changes is that I am not too keen on having some chart editor dialogs be accessible easily through the ui folder. I am aware this was done in order for RuntimeComponentBuilder to work but making them public is just going to encourage tampering with xml dialog files, which will either have little to no effect on programmed functionality or just lead to a bunch of crashes due to missing components/invalid xml structure. This has bothered me since the launch of 0.3

also rest in peace directory field from stage datas you were made obsolete

The macro component builder is way more stable, the only reason any of the chart editor components are in the UI folder is because they haven't been migrated yet.

Also, those XML files were in the assets/data/ui folder before, so this complaint is already relevant on release builds of the game.

Of note, I do feel like song audio should be separated into folders with their variation names, since it could get somewhat messy if a lot of variations for a specific song exist.

My main complaint with this is that it breaks older .fnfc files and we would have to support flat files anyway to allow them to work.

@KoloInDaCrib
Copy link

My main complaint with this is that it breaks older .fnfc files and we would have to support flat files anyway to allow them to work.

Tangentially related, but I'm not sure if having .fnfc files be loaded as is instead of having to be renamed into .zip and then extracted would be a good compromise.
On one hand, it removes the aforementioned somewhat complicated steps and it also technically solves the problem of poor organization, since most of the song files (only exception being optional scripts) are within the .fnfc file.
On the other hand this would remove the modding capability of changing one file (for instance you wouldn't be able to only change the metadata file but instead would need to change the entire .fnfc file), as well as pre-caching assets would be slightly more complicated.

@AbnormalPoof
Copy link
Collaborator

My main complaint with this is that it breaks older .fnfc files and we would have to support flat files anyway to allow them to work.

What about having flat files for .fnfc only, and variation folders for when the song is implemented in-game?

Another solution I can think of is that the game can recursively check for charts/audio, so the charts and songs can be organized the way the modder deems fit.

@EliteMasterEric
Copy link
Member Author

My main complaint with this is that it breaks older .fnfc files and we would have to support flat files anyway to allow them to work.

Tangentially related, but I'm not sure if having .fnfc files be loaded as is instead of having to be renamed into .zip and then extracted would be a good compromise. On one hand, it removes the aforementioned somewhat complicated steps and it also technically solves the problem of poor organization, since most of the song files (only exception being optional scripts) are within the .fnfc file. On the other hand this would remove the modding capability of changing one file (for instance you wouldn't be able to only change the metadata file but instead would need to change the entire .fnfc file), as well as pre-caching assets would be slightly more complicated.

These concerns are the main reason I haven't put much work into trying to load FNFC files from the game folder. I do really like the idea in concept though.

At one point, I imagined the game might eventually get a "Mod Editor" where you could build a manifest and organize the assets for your mods via a HaxeUI user interface, and could add assets by dragging them in. This would obviously support FNFCs.

@KoloInDaCrib
Copy link

At one point, I imagined the game might eventually get a "Mod Editor" where you could build a manifest and organize the assets for your mods via a HaxeUI user interface, and could add assets by dragging them in. This would obviously support FNFCs.

Perhaps the aforementioned editor could be bundled with the Mod Menu, similar to the one lemz1 made and pr'd. This would definitely be helpful to those that are new in the modding scene.
Though not entirely sure if funkin needs another editor - I am of the opinion that three editors (chart, stage and character/player) would be more than enough, and the mod editor would be somewhat useless because of the ability to manually make folders and move files there.

@EliteMasterEric
Copy link
Member Author

the mod editor would be somewhat useless because of the ability to manually make folders and move files there.

Aside from letting you easily edit the manifest for your mod without breaking it, the key idea of the Mod Editor is that it would be a central hub for content creation, and would integrate closely with the Character, Stage, and Song editors. It'd be impossible to make something to create complex mods, but for simple mods that don't need scripts (just a custom character singing a custom song in a custom stage), I don't think people should need to do a bunch of fiddling with folders and scripts.

Pushing the hurdle for content creation as low as possible is important to encourage aspiring creators to experiment.

Copy link
Member Author

Choose a reason for hiding this comment

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

lol nobody even noticed this one

@doggogit
Copy link
Contributor

@EliteMasterEric @ninjamuffin99

One thing I'd like to request from the Crew, from the bottom of my heart, from the very core of my soul, is to not force this onto mod developers.

I want to make myself clear, any mods that I make, I'll maintain, however, I do NOT want to be forced to use this system on mods that have existed before this gets merged.

This system might also configure the way Paths work as well, but please, this is a MAJOR BREAKING CHANGE and a lot of mod developers will feel discouraged to keep maintaining their projects when something this large hits the game and all of a sudden all mods break and people are scrambling to make it work.

I'm not discouraging The Great Sort, I'm a fan of it (really!), however, please have the older Paths system still in place for people using the embedded mod folder.

I want mods to last and have some sort of backwards compatibility. Breaking changes, this often, this early, shouldn't be forced onto people.

@AbnormalPoof
Copy link
Collaborator

This completely restructures the game's filesystem. I can't really see a way of implementing backwards compatibility.

@doggogit
Copy link
Contributor

This completely restructures the game's filesystem. I can't really see a way of implementing backwards compatibility.

My bad, I didn't mean to lash out that much, but, this'll be a maintainers nightmare, so I think we should have it marinate a bit before it gets pushed, maybe not for 0.6 but for future versions like a year down the line, perhaps, or have builds that modders can use and test out.

@EliteMasterEric
Copy link
Member Author

The new file structure is something that I've wanted to implement for a while. The existing asset system causes many bugs (such as the bug where certain stages don't work in the Chart Editor) and was a major obstacle in implementing the performance improvements I've wanted to make.

Aside from the new file structure being more intuitive than the current one (Where do the stage assets go? That's right, they go in the stage folder!), I plan on making a migration guide over here.

Ultimately, it's important to understand that the game is in an early access state and subject to change; we'd be more paranoid about making things backwards-compatible if the game in a fully release state.

By the way, these changes are slated for v0.7 (the update after the next pit stop), not v0.6.

@doggogit
Copy link
Contributor

The new file structure is something that I've wanted to implement for a while. The existing asset system causes many bugs (such as the bug where certain stages don't work in the Chart Editor) and was a major obstacle in implementing the performance improvements I've wanted to make.

Aside from the new file structure being more intuitive than the current one (Where do the stage assets go? That's right, they go in the stage folder!), I plan on making a migration guide over here.

Ultimately, it's important to understand that the game is in an early access state and subject to change; we'd be more paranoid about making things backwards-compatible if the game in a fully release state.

By the way, these changes are slated for v0.7 (the update after the next pit stop), not v0.6.

Thanks for the clarification, and thank you for your efforts to document such changes.

Are you considering making internal tools (for e.g., a script or some built-in converter) or just the migration documentation?

@AbnormalPoof
Copy link
Collaborator

AbnormalPoof commented Jan 24, 2025

Some more questions I have about this PR:

  1. Is there a comparison in load times between this and the current asset system? (i.e loading into a week with a lot of assets like WeekEnd 1)
  2. Since Paths returns an AssetPath object now, will scripts that use Paths have to be updated in a significant way?
  3. What would the file structure of a Polymod mod look like with this new system?

@EliteMasterEric
Copy link
Member Author

The file structure of a Polymod mod will look like the file structure as the base game, as it did before.

For loading times, I think they're a little longer right now but it displays a loading screen with progress bar rather than freezing the game, and it's a good place for optimization once it's 100% working.

Since Paths returns an AssetPath object now, will scripts that use Paths have to be updated in a significant way?

Yes, however my reasoning behind this is:

  1. Any bits of code that used Paths functions before shouldn't need to change, since the functions that used to take a String path now take an AssetPath and the functions that used to provide a String now provide an AssetPath. This just mandates that everything use the Paths functions for additional validation and the like.

  2. Any stage, character, or song which uses additional assets (for example, Week 3's train plays a sound file) needs to register those assets beforehand via overriding the queryAssets function; if they don't, the game will stutter on release builds and throw an error on debug builds. Since this is a breaking change already, I decided to bundle a bunch of other breaking changes I wanted to make along with it, so we have one large breaking update rather than three smaller ones.

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.

7 participants