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

Added basePath support to SpriteSheetLoader #169

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nki2
Copy link

@nki2 nki2 commented Nov 19, 2015

I have added basePath support to SpriteSheetLoader,
because the current SpriteSheetLoader doesn't take basePath into account when loading spritesheets using SpriteSheetLoader.

@endel
Copy link

endel commented Oct 14, 2016

Would be quite convenient to have this.

@lannymcnie
Copy link
Member

Thanks, this is definitely needed. I want to do some testing before accepting the PR, but your work is appreciated.

Our plan is to remove this requirement by getting rid of sub-queues, and instead pushing manifest and SpriteSheetLoader contents into the main queue so that things like maxConnections can be applied to them.

Cheers.

@lannymcnie
Copy link
Member

lannymcnie commented Oct 27, 2016

Not super happy with the ambiguity with how this handles basepath on images. Right now the item.path is used for the basepath of the internal manifest instead of the new basepath parameter that is passed in.

Not sure this makes a ton of sense, and it might be better to instead just assume the same basepath for sub-items. Prepending the basepath could result in some weird behavior:

For example if you load "spritesheet.json" with a path of "sprites/" and a basepath of "site/assets/" then you will end up with
sprites/site/assets/image.png
instead of
site/assets/sprites/image.png

Ideally the SpriteSheet format should support a path property for it's assets (instead of the loadItem), and that could be used on individual items or something, and the basepath could be propagated properly. Either way, I am not sold on this approach.

@lannymcnie
Copy link
Member

This will be considered in PreloadJS 2.0

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.

3 participants