Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Change AssetPaths args #2575

Merged
merged 12 commits into from
Nov 18, 2022
Merged

Conversation

Geokureli
Copy link
Member

@Geokureli Geokureli commented Jun 3, 2022

fixes #2547

AssetPaths has always been a clunky system, whenever people come to me with issues regarding it, I tell them just to delete it and use String paths.

Problems

  • Duplicate filenames break down the entire system. even when they are in different directories, or if one of them is excluded via Project.xml
  • Custom rename arg only takes the file name, and ignores the directory

Solutions

  • Allow duplicate field names, warn the user and omit the duplicated field
  • Change filterExtensions to an include/exclude regular expression (or wildcard strings), allowing exclusion for any context.
  • Change custom renaming to take the entire filepath, not just name and extension

Excerpt from the 5.0.0 migration guide

AssetPaths (or any class built using FlxAssets.buildFileReferences) will no longer throw an error when two files have the same file name. Now it will give a warning and ignore whichever file is nested more deeply in the folder, or whichever file was found later.

The filterExtensions arg was removed and replaced with include and exclude args. These new args can either be an EReg, or a wildcard string, similar to openfl's project.xml args. For example to exclude everything in a folder called "test", as well as any .psd files, you would either use "*/test/*|*.ase" or ~/\/test\/|\.ase/.

Lastly, the rename arg was changed, significantly. Custom renaming is already a relatively new and unknown feature of AssetPaths, but now it may be the most powerful! The rename arg is a function that will take a filepath (a relative filepath from the project.xml) and returns a field name used to access that path. The previous versions there would be no way to include two files with the same name, for instance, "assets/images/mario/walk.png" and "assets/images/luigi/walk.png" wouldn't be allowed, now you can call them mario_walk.png and luigi_walk.png. Additionally, you can return null to exclude a specific file.

@Geokureli Geokureli marked this pull request as draft June 3, 2022 18:20
This way users aren't forced to use regexes, but can opt in
if they want to. Plus we don't break backwards compatibility.
@Geokureli
Copy link
Member Author

Geokureli commented Jun 3, 2022

@player-03

Plus your exclude example should be "/exclude/|\.ogg$".

"\.ogg$" is correct, I'll change
"/exclude" is actually preferred since it will skip the directory, where "/exclude/" will recursively scan the directory and exclude all files within it. I'm still thinking about how to explain that. ("/exclude$" would be best but I thought it would be confusing)

The old version, though not very flexible, was easy for new programmers to understand. Jumping straight from there to regular expressions may scare them away.

IMO the old system was practically unusable, it was rare that this tool was actually worthwhile. There's still some stuff I want to try and do to improve this. but even with these changes my gut says to just remove AssetPaths from the templates/demos and reinforce the usage of string paths, which is even easier for new programmers to understand, imo. especially since VSC has "right click->copy relative path"

@Geokureli
Copy link
Member Author

that said, input is absolutely welcome as I'm still not 100% sure what the best solution is. So far this is my best guess

@player-03
Copy link
Contributor

I'd say the best solution is to allow both wildcard strings and regexes. To distinguish, just check if the user wrote "*.gif" or ~/\.gif$/. Yes it's possible; see my pull request.

player-03 and others added 4 commits June 3, 2022 14:38
Co-authored-by: George FunBook <gkurelic@gmail.com>
Co-authored-by: George FunBook <gkurelic@gmail.com>
Add more options for asset path args.
@Geokureli
Copy link
Member Author

Another thing I was wondering, since include/exclude will always have to match the Project.xml include/excludes, is it possible to auto-apply those filters and only check the files that will actually be exported?

Even if we could, I wonder if there are drawbacks to this.
Would it bog down compile time?
Is it worth forcing everyone to wrap AssetPaths.some_file__ogg refs in #if desktop blocks?

@Geokureli Geokureli marked this pull request as ready for review November 18, 2022 00:58
@Geokureli Geokureli changed the title change asset path args Change AssetPaths args Nov 18, 2022
@Geokureli Geokureli merged commit 9d47edb into HaxeFlixel:dev Nov 18, 2022
Geokureli added a commit that referenced this pull request Nov 18, 2022
@Geokureli Geokureli deleted the assetpaths_overhaul branch August 9, 2023 15:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AssetPaths changes
2 participants