[Feat] Normalize paths or allow asset path customization

[focustense]

I'm using bevy_asset_loader to preload all assets, and currently keeping LDtk files in assets/levels adjacent to other paths like assets/spritesheets. The reasons for the latter aren't absolutely critical, but (a) multi-worlds feature is still considered experimental, (b) I'd like to support "external" mod-worlds, and (c) it helps to keep things organized in general.

Anyway, while working on a feature today that depends on shared assets being... well, shared, I discovered that bevy_ecs_ldtk was creating a second copy of all the assets referenced in the LDtk project. This is because the LDtk paths are relative to the project file, and when passed to the asset loader, are in the form of levels\\../spritesheets/foo.png. These paths do load, of course, but Bevy's asset loader assigns them new AssetPathIds despite pointing to the same file as spritesheets/foo.png, so they get loaded and added as new assets.

I realize that in theory there's a lot of madness around symlinks, and other very good reasons why all the pieces involved behave as they do. Nevertheless, in practice, and certainly in my project, those edge cases are never going to happen, and what I do care about is making sure that the LDtk systems and non-LDtk systems get the same asset when they request the same asset.

Slapping a path-clean on the ldtk_path_to_asset_path method solves the problem beautifully for me, but requires a change directly to the source code; I can't intercept this in the app code. Perhaps it is too blunt an instrument to apply to every user - maybe some people really do care about nuances like symlinks - so if that is the case, then I'd like to request canonicalization as an opt-in feature, whether it's something simple like a flag in LdtkSettings or requires us to do more of the legwork e.g. by implementing some trait with the actual conversion function.

It's probably a weird edge case and I don't expect it to get high priority, given that the workaround is technically trivial (just make sure all LDtk projects are in the asset root), but wanted to bring it up particularly because there's no warning that it's happening. My project is still small, but in a project with hundreds of MBs of assets, it could end up doubling the loading time and wasting a lot of memory. Also, bevy_ecs_ldtk having dedicated code to resolve LDtk asset paths suggests that it's designed to accommodate these scenarios, and does handle them, but won't play nice with other systems unless the paths are normalized.

[Trouv]

Great issue! I had never considered that the AssetPathIds would be different and cause the asset server to load assets twice, but it makes a lot of sense. This is probably affecting my projects as well, I just hadn't noticed.

Perhaps it is too blunt an instrument to apply to every user

I'm not so sure about this. I think having shared assets is probably a much more common use case than symlinks. I would be welcome to a PR that applies a standardization to the generated paths, perhaps via path-clean. I doubt this would pose a problem for people, but if it does they could file an issue and we could patch in a more nuanced approach, perhaps via a cargo feature. For the time being, I mostly see this as an improvement that is unlikely to affect users negatively. Trying to implement a more complicated solution before anybody expresses they need it is probably premature. Idk maybe I underestimate the commonality of symlinks in game dev.

That being said, you may want to wait a day or two before submittting a PR for this. I'm currently merging a couple months worth of breaking changes to the assets module, so merge conflicts would be likely.

[focustense]

I'm always cautious about suggesting non-backward-compatible changes to projects that I only have a passing familiarity with - you never know who's depending on the old behavior or who has some seriously bizarre path structure that doesn't work with normalized paths.

But, if you're alright with making it a global change, then it's really a very simple 1-line change to ldtk_path_to_asset_path (technically, more than 1 line because of rustfmt). Should be a link to my commit just below my OP. There's already a merge conflict from v0.8.0, since it's no longer in src/assets.rs, it's now in src/assets/mod.rs, but the function itself looks unchanged as of right now.

I can merge it back into mainline and do a PR whenever you're ready. Let me know if it needs tests, or anything like that.

[Trouv]

I'm always cautious about suggesting non-backward-compatible changes to projects that I only have a passing familiarity with

That's a good approach, I appreciate it.

it's now in src/assets/mod.rs, but the function itself looks unchanged as of right now.

It will move to src/assets/ldtk_project.rs in a PR relatively soon, but I don't think it has changed still logically. I'll comment here when this is merged.

Let me know if it needs tests, or anything like that.

Tests wouldn't hurt, since the function is getting slightly more complex. At the very least it would demonstrate in the PR how the behavior has changed

[Trouv]

The asset rework has been merged #244

[focustense]

That's some rework. Looks like all that code I wrote depending on Handle<LdtkLevel> is going to break.

Anyway, PR is out. Feels a little odd writing tests for an internal helper function, but it's probably the best I can do unless/until the project is set up for integration testing; testing an entire AssetLoader is pretty inconvenient unless one is willing to spin up an entire Bevy app and feed it a real asset.