Setup basic logging

[torkleyy]

TODO: decide on a simple logging implementation and initialize it in the Application

---

This change is 

[Rhuagh]

Reviewed 20 of 20 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[Rhuagh]

Reviewed 20 of 20 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[Xaeroxe]

I like this a lot! Thank you!

Question for @amethyst/engine-devs and @Rhuagh though, A lot of our crates are starting to get these common dependencies, such as log and shrev. Perhaps we should identify our "framework dependencies" and re-export them in core just to make maintenance easier. (Much like I did with cgmath)

[Xaeroxe]

I like this a lot! Thank you!

Question for @amethyst/engine-devs and @Rhuagh though, A lot of our crates are starting to get these common dependencies, such as log and shrev. Perhaps we should identify our "framework dependencies" and re-export them in core just to make maintenance easier. (Much like I did with cgmath)

[torkleyy]

We have to be careful though because if we do this with private dependencies (which log is) we would have to make a breaking change when we upgrade them.

[torkleyy]

We have to be careful though because if we do this with private dependencies (which log is) we would have to make a breaking change when we upgrade them.

[torkleyy]

So..any suggestions for the logger implementation?

[torkleyy]

So..any suggestions for the logger implementation?

[Xaeroxe]

I'm content with the idea of having to make breaking changes when upgrading log, chances are pretty good users are going to want to log their own things too. (I know I've done this for a few games I've worked on)

[Xaeroxe]

I'm content with the idea of having to make breaking changes when upgrading log, chances are pretty good users are going to want to log their own things too. (I know I've done this for a few games I've worked on)

[torkleyy]

Ah, nicely observed @Xaeroxe. I think it's okay to expose the log crate then 👍

[torkleyy]

Ah, nicely observed @Xaeroxe. I think it's okay to expose the log crate then 👍

[Emilgardis]

Related
#124
#460

[Emilgardis]

Related
#124
#460

[Rhuagh]

simple_logger for examples imho.

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[Rhuagh]

simple_logger for examples imho.

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[torkleyy]

Only problem with that is that the last commit was two years ago :( It looks pretty much like env_logger, so we could as well just use that.

[torkleyy]

Only problem with that is that the last commit was two years ago :( It looks pretty much like env_logger, so we could as well just use that.

[Rhuagh]

That or simplelog, I don't mind either. For production games it will likely be log4rs or similar

[Rhuagh]

That or simplelog, I don't mind either. For production games it will likely be log4rs or similar

[Rhuagh]

fern looks nice imho

[Rhuagh]

fern looks nice imho

[Rhuagh]

Use env_logger for the examples. How far is this from completion?

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[Rhuagh]

Use env_logger for the examples. How far is this from completion?

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[torkleyy]

This is pretty easy to complete, and I planned to use fern. Just didn't have time for it yet.

[torkleyy]

This is pretty easy to complete, and I planned to use fern. Just didn't have time for it yet.

[Rhuagh]

Ok. Use fern, I like that one the best :)

[Rhuagh]

Ok. Use fern, I like that one the best :)

[Xaeroxe]

Bad robot

[Xaeroxe]

Bad robot

[torkleyy]

Example output:

[amethyst::app][INFO] Initializing Amethyst...
[amethyst::app][INFO] Version: v0.5.1-398-g40fa47b
[amethyst::app][INFO] Platform: x86_64-unknown-linux-gnu
[amethyst::app][INFO] Amethyst git commit: 40fa47ba289cc87f5fc927a2939e071adff21af0
[amethyst::app][INFO] Rustc version: 1.25.0-nightly Nightly
[amethyst::app][INFO] Rustc git commit: 51b0b3734cbd0ca58c8be3512d53fce2d95f40dd
[amethyst::app][INFO] Engine is shutting down

Color may be added later.

[torkleyy]

Example output:

[amethyst::app][INFO] Initializing Amethyst...
[amethyst::app][INFO] Version: v0.5.1-398-g40fa47b
[amethyst::app][INFO] Platform: x86_64-unknown-linux-gnu
[amethyst::app][INFO] Amethyst git commit: 40fa47ba289cc87f5fc927a2939e071adff21af0
[amethyst::app][INFO] Rustc version: 1.25.0-nightly Nightly
[amethyst::app][INFO] Rustc git commit: 51b0b3734cbd0ca58c8be3512d53fce2d95f40dd
[amethyst::app][INFO] Engine is shutting down

Color may be added later.

[torkleyy]

I'd say this is ready for merge; we want to add a logging config later, but we can now finally use logging when creating a PR and don't have to use printlns and remove them at the end :)

[torkleyy]

I'd say this is ready for merge; we want to add a logging config later, but we can now finally use logging when creating a PR and don't have to use printlns and remove them at the end :)

[omni-viral]

How user may control where those log goes?

[torkleyy]

Atm this will just log to stdout. As I said above, we need to add a logging configuration, but this PR is helpful as is.

[torkleyy]

Atm this will just log to stdout. As I said above, we need to add a logging configuration, but this PR is helpful as is.

[Emilgardis]

An easy way to make this configurable for now is to add one function to App and change App::new

• App::new: fn new<P: AsRef<Path>>(path: P, initial_state: T) -> Result<Self>
  uses App::with_logger internally, using the default logger
• App::with_logger: fn with_logger<P: AsRef<Path>>(path: P, initial_state: T, logger: fern::Dispatch) -> Result<Self>

[Emilgardis]

An easy way to make this configurable for now is to add one function to App and change App::new

• App::new: fn new<P: AsRef<Path>>(path: P, initial_state: T) -> Result<Self>
  uses App::with_logger internally, using the default logger
• App::with_logger: fn with_logger<P: AsRef<Path>>(path: P, initial_state: T, logger: fern::Dispatch) -> Result<Self>

[Xaeroxe]

This is pretty good! I just have a couple nits.

[Xaeroxe]

Why was this change made?

[torkleyy]

I forgot to reset it after running the example (glutin bug currently causes a crash when using vsync).

[Xaeroxe]

Are we sure we want this? That's rather a lot of logging for anyone who subscribes to trace events. Advancing the frame is somewhat mundane and not really noteworthy.

[torkleyy]

Well you usually filter the logging at this level (if you don't, you shouldn't expect to get only a few lines of output). The whole point of trace is to provide information you'll almost never need, but can be quite helpful to localize a bug in very rare cases.

[Xaeroxe]

Ok I understand now. Thanks!

[Rhuagh]

Reviewed 2 of 6 files at r2, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

src/app.rs, line 316 at r3 (raw file):

        use rustc_version_runtime;

        fern::Dispatch::new()

Add a with_logger function on ApplicationBuilder, and use this as the fallback in the build function?

---

Comments from Reviewable

[Rhuagh]

Reviewed 2 of 6 files at r2, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

src/app.rs, line 316 at r3 (raw file):

        use rustc_version_runtime;

        fern::Dispatch::new()

Add a with_logger function on ApplicationBuilder, and use this as the fallback in the build function?

---

Comments from Reviewable

[Rhuagh]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

src/app.rs, line 316 at r3 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Add a with_logger function on ApplicationBuilder, and use this as the fallback in the build function?

Actually, just put this in a with_default_logger(mut self) -> Self {}

---

Comments from Reviewable

[Rhuagh]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

src/app.rs, line 316 at r3 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Add a with_logger function on ApplicationBuilder, and use this as the fallback in the build function?

Actually, just put this in a with_default_logger(mut self) -> Self {}

---

Comments from Reviewable

[torkleyy]

Problem is we don't get all the initialization messages if the logger setup is in a builder method.

[torkleyy]

Problem is we don't get all the initialization messages if the logger setup is in a builder method.

[Rhuagh]

Hrmph, I guess that's true.

[Rhuagh]

Hrmph, I guess that's true.

[Xaeroxe]

LGTM! Thanks!

bors r+

[bors]

Timed out

[bors]

Timed out

[Xaeroxe]

Reviewed 13 of 20 files at r1, 2 of 6 files at r2, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[Xaeroxe]

Reviewed 13 of 20 files at r1, 2 of 6 files at r2, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[Xaeroxe]

Overriding

[Xaeroxe]

Overriding

[Emilgardis]

This needs to be documented somewhere. Even though it is roughly implemented, making an effort to document it should make it clearer on how to do this properly.

[Emilgardis]

This needs to be documented somewhere. Even though it is roughly implemented, making an effort to document it should make it clearer on how to do this properly.