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

Make crates more self-contained and introduce `amethyst_core` #396

Closed
torkleyy opened this Issue Oct 8, 2017 · 20 comments

Comments

Projects
None yet
5 participants
@torkleyy
Member

torkleyy commented Oct 8, 2017

A few days ago I came up with an idea, hey why don't we split up Amethyst and make the creates just utility crates for Specs. While some liked the idea, @Xaeroxe was quite skeptical. And I share that skepticism. That's why I came up with a compromise:

Right now we have our crates amethyst_assets, amethyst_renderer, both of which contain their own functionality but are then wrapped in the main crate. We should instead put everything related to assets in the assets crate and the same for the renderer. This would not only solve constant complications with the orphan rules, but also allow us to make those crates optional, while still allowing easy integration and a cohesive experience thanks to the concept of ECS. Now, there are however things many Amethyst crates will share, e.g. Transform. These things should go into amethyst_core.

What should happen to the main crate?

I'm not sure if a main crate should remain, I've experimented with making a more generic state machine lately and the ecs module would essentially get dropped. Timer stuff probably goes into the core crate and fps counters and profiling tools could go into an amethyst_tools crate.

@omni-viral

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Oct 8, 2017

Member

The link is slightly broken

Member

omni-viral commented Oct 8, 2017

The link is slightly broken

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 8, 2017

Member

Fixed now, sorry

Member

torkleyy commented Oct 8, 2017

Fixed now, sorry

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 8, 2017

Member

Yay!

Member

Rhuagh commented Oct 8, 2017

Yay!

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 8, 2017

Member

@torkleyy I guess I'm not entirely certain how this proposal solves our orphan rules problem. The purpose of the renderer crate is to render assets on screen, while the purpose of the assets crate is to allow new asset types to be defined. So when @omni-viral wants to implement amethyst_assets::Asset for amethyst_renderer::Mesh I'm still not sure how he'd do that under this proposal. Does the proposal just accept such an implementation won't be possible? Because if so we have two choices:

  • Wrap Mesh ourselves in amethyst_core and implement Asset for the wrapper (similar to what we've already done)
  • Ask downstream users to do the same for their own projects, which is essentially asking them to write the same piece of code for every project just to satisfy our need to separate concerns.

Basically I'm questioning the wisdom of trying to separate concerns too heavily, because these concerns might not be that separate after all.

Member

Xaeroxe commented Oct 8, 2017

@torkleyy I guess I'm not entirely certain how this proposal solves our orphan rules problem. The purpose of the renderer crate is to render assets on screen, while the purpose of the assets crate is to allow new asset types to be defined. So when @omni-viral wants to implement amethyst_assets::Asset for amethyst_renderer::Mesh I'm still not sure how he'd do that under this proposal. Does the proposal just accept such an implementation won't be possible? Because if so we have two choices:

  • Wrap Mesh ourselves in amethyst_core and implement Asset for the wrapper (similar to what we've already done)
  • Ask downstream users to do the same for their own projects, which is essentially asking them to write the same piece of code for every project just to satisfy our need to separate concerns.

Basically I'm questioning the wisdom of trying to separate concerns too heavily, because these concerns might not be that separate after all.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 8, 2017

Member

Why not make amethyst_renderer dependent on amethyet_assets?

Member

torkleyy commented Oct 8, 2017

Why not make amethyst_renderer dependent on amethyet_assets?

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 8, 2017

Member

That works. That was a relatively straightforward fix haha :) Then sure I have no problem with this if we're allowing amethyst_* crates to depend on amethyst_* crates other than amethyst_core

Member

Xaeroxe commented Oct 8, 2017

That works. That was a relatively straightforward fix haha :) Then sure I have no problem with this if we're allowing amethyst_* crates to depend on amethyst_* crates other than amethyst_core

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 8, 2017

Member

Ofc, that would be a given, my coming amethyst_gltf would depend on both assets and renderer, as an example.

Member

Rhuagh commented Oct 8, 2017

Ofc, that would be a given, my coming amethyst_gltf would depend on both assets and renderer, as an example.

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 8, 2017

Member

Btw, we will need to look into error-chain or something like it from the get-go when we do this (we should anyway).

And app.rs, where does that go, amethyst_core or its own crate ?

Member

Rhuagh commented Oct 8, 2017

Btw, we will need to look into error-chain or something like it from the get-go when we do this (we should anyway).

And app.rs, where does that go, amethyst_core or its own crate ?

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 8, 2017

Member

That's a tough question:

  1. I wanted to add a similar thing to Specs, but have to wait for specialization
  2. Application is incompatible with ParSeq
  3. The way it currently works conflicts with the plans to give more freedom wrt dispatchers and World
Member

torkleyy commented Oct 8, 2017

That's a tough question:

  1. I wanted to add a similar thing to Specs, but have to wait for specialization
  2. Application is incompatible with ParSeq
  3. The way it currently works conflicts with the plans to give more freedom wrt dispatchers and World
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 8, 2017

Member

Yeah, I kinda felt that would be the case, so my suggestion is to put it in it's own crate, so it's easy to swap out. Something like amethyst_app.

Member

Rhuagh commented Oct 8, 2017

Yeah, I kinda felt that would be the case, so my suggestion is to put it in it's own crate, so it's easy to swap out. Something like amethyst_app.

@Binero

This comment has been minimized.

Show comment
Hide comment
@Binero

Binero Oct 10, 2017

Contributor

I think a main crate should definitely remain. When splitting up amethyst in modules, I think it's important to have the main crate to bring them all together, and to allow the user to quickly get started without too much puzzling.

Contributor

Binero commented Oct 10, 2017

I think a main crate should definitely remain. When splitting up amethyst in modules, I think it's important to have the main crate to bring them all together, and to allow the user to quickly get started without too much puzzling.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 10, 2017

Member

@Binero My point is that it's not harder and by making them modular, docs can be more focused.

I mean how is

extern crate amethyst_renderer;
extern crate amethyst_physics;

harder than

extern crate amethyst;

use amethyst::renderer::...;
use amethyst::physics::...;

? We just provide a nice example and explanation to get started. This isn't what I'd call puzzling. Additionally, these crates should require zero glue code.

Member

torkleyy commented Oct 10, 2017

@Binero My point is that it's not harder and by making them modular, docs can be more focused.

I mean how is

extern crate amethyst_renderer;
extern crate amethyst_physics;

harder than

extern crate amethyst;

use amethyst::renderer::...;
use amethyst::physics::...;

? We just provide a nice example and explanation to get started. This isn't what I'd call puzzling. Additionally, these crates should require zero glue code.

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 10, 2017

Member

these crates should require zero glue code.

❤️ You have no idea how happy that sentence makes me

Member

Xaeroxe commented Oct 10, 2017

these crates should require zero glue code.

❤️ You have no idea how happy that sentence makes me

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 10, 2017

Member

With the exception of a single function call or two to register things in world/dispatch, but that's user code anyway

Member

Rhuagh commented Oct 10, 2017

With the exception of a single function call or two to register things in world/dispatch, but that's user code anyway

@amethyst amethyst deleted a comment from Rhuagh Oct 10, 2017

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 10, 2017

Member

(he accidentally submitted his comment twice so I deleted one of them. I'm not trying to censor things I swear :P)

Member

Xaeroxe commented Oct 10, 2017

(he accidentally submitted his comment twice so I deleted one of them. I'm not trying to censor things I swear :P)

@Binero

This comment has been minimized.

Show comment
Hide comment
@Binero

Binero Oct 10, 2017

Contributor

@torkleyy It makes it incredibly hard to read documentation, it requires the user to manage all dependencies and is non-obvious.

Contributor

Binero commented Oct 10, 2017

@torkleyy It makes it incredibly hard to read documentation, it requires the user to manage all dependencies and is non-obvious.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 11, 2017

Member

@Binero How about that:

We keep the main crate for now, which makes sense anyways because it's easier to refactor. Then, after all the other changes are made, we revisit this again and make a decision.

Member

torkleyy commented Oct 11, 2017

@Binero How about that:

We keep the main crate for now, which makes sense anyways because it's easier to refactor. Then, after all the other changes are made, we revisit this again and make a decision.

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 11, 2017

Member

Yeah, I'm fine with that. If it just becomes a long list of "pub extern crate amethyst_$t as $t;" so be it :)

Member

Rhuagh commented Oct 11, 2017

Yeah, I'm fine with that. If it just becomes a long list of "pub extern crate amethyst_$t as $t;" so be it :)

bors bot added a commit that referenced this issue Oct 12, 2017

Merge #405
405: [rs] 01: Decentralize asset formats r=Xaeroxe a=torkleyy

This PR (and several follow-ups) will slowly restructure Amethyst according to #396 

## Improvements

* Don't wrap crates in the main crate (instead, more self-contained)
* `amethyst_renderer` now depends on `amethyst_assets`
@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 17, 2017

Member

So @Rhuagh did #415 and #419 . Thank you!

Member

torkleyy commented Oct 17, 2017

So @Rhuagh did #415 and #419 . Thank you!

bors bot added a commit that referenced this issue Oct 22, 2017

Merge #438
438: [rs] 09: Flatten crate structure of amethyst_renderer r=Rhuagh a=torkleyy

Partially addresses #396

bors bot added a commit that referenced this issue Oct 22, 2017

Merge #438
438: [rs] 09: Flatten crate structure of amethyst_renderer r=torkleyy a=torkleyy

Partially addresses #396
@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 25, 2017

Member

I think we're done here. Re-open if you disagree with me.

Member

Xaeroxe commented Oct 25, 2017

I think we're done here. Re-open if you disagree with me.

@Xaeroxe Xaeroxe closed this Oct 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment