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

The examples readme doesn't explain how to run the examples properly #472

Closed
FSMaxB opened this issue Sep 10, 2020 · 9 comments · Fixed by #478
Closed

The examples readme doesn't explain how to run the examples properly #472

FSMaxB opened this issue Sep 10, 2020 · 9 comments · Fixed by #478
Labels
C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation C-Examples An addition or correction to our examples

Comments

@FSMaxB
Copy link
Contributor

FSMaxB commented Sep 10, 2020

The readme is missing the information that you need to run the cargo commands from the project root.

I found this out because even though the sprite example runs when running cargo from the examples directory, it doesn't actually display the sprite because the relative path is from the project root, so it needs to be run from the project root.

It would be great if this could be explained in the examples README.

@karroffel karroffel added C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation C-Examples An addition or correction to our examples labels Sep 10, 2020
@lberrymage
Copy link
Contributor

This should be closed in light of #471 merging

@cart
Copy link
Member

cart commented Sep 11, 2020

Good call!

@cart cart closed this as completed Sep 11, 2020
@cart cart reopened this Sep 11, 2020
@cart
Copy link
Member

cart commented Sep 11, 2020

Actually wait this seems like a separate issue. That pr fixes a broken command. This issue points out that assets only work correctly when run from the project root.

@lberrymage
Copy link
Contributor

Oh gotcha gotcha. I can work on fixing that; we probably only need to add

env!("CARGO_MANIFEST_DIR")

to the asset paths.

@cart
Copy link
Member

cart commented Sep 11, 2020

interestingly we already do that for the root path:

image

Although I think someone mentioned that we don't currently use root path when loading assets directly 😄

@lberrymage
Copy link
Contributor

Huh... I don't if we would want to use the root path in case the user wants to load from an arbitrary directory, but also I don't know if the asset server is intended to be used with a single asset directory. That would make the most sense from a usage perspective at least; a change in .load()s behavior might not actually break that much since people typically have all their assets in one directory.

Not sure if you were implying that we might want to change AssetServer::load()s behavior, but I thought I'd bring it up in case you were leaning in that direction. Of course it's just as easy to manually load from CARGO_MANIFEST_DIR in the examples to solve this particular problem 😉.

@cart
Copy link
Member

cart commented Sep 11, 2020

I think we actually want well-defined (or at least scoped) asset paths, as some platforms require that. I think its natural to restrict loads to the root path.

@lberrymage
Copy link
Contributor

That makes sense. I'll can whip up a PR pretty soon here.

@FSMaxB
Copy link
Contributor Author

FSMaxB commented Sep 12, 2020

This is great. At first I just had a change to the README in mind, but this fix is much better!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Bug An unexpected or incorrect behavior C-Docs An addition or correction to our documentation C-Examples An addition or correction to our examples
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants