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

Renderer rewrite #285

Merged
merged 123 commits into from Aug 27, 2017

Conversation

@torkleyy
Member

torkleyy commented Aug 15, 2017

This is the PR merging renderer into develop. It is WIP, but shows the overall progress.

TODO

  • Fix examples
  • Check docs, crate structure
  • Properly handle configs

ebkalderon added some commits Mar 7, 2017

Update dependencies, add cam.rs
Add several static methods to Rgba

Allow color::Rgba to be used in constant buffers

Rename encoder.rs to enc.rs, update Encoder type

Allow light types to be uploaded as constant buffers

Add &Scene to PostprocFn, add LightIter and MeshIter to Scene type

Derive Serialize, Deserialize for TextureBuilder
+ if self.config.fullscreen {
+ wb = wb.with_fullscreen(winit::get_primary_monitor());
+ }
+ match self.config.dimensions {

This comment has been minimized.

@stenverbois

stenverbois Aug 23, 2017

Contributor

Nit: these could be made a bit cleaner with if let bindings.

@stenverbois

stenverbois Aug 23, 2017

Contributor

Nit: these could be made a bit cleaner with if let bindings.

Xaeroxe and others added some commits Aug 23, 2017

Made the assets example compile.
This moves the assets example to the new renderer as well as to the new input handler (although it does not use the new `actions` system yet). It still
crashes shortly after starting up, because of some resources/components not being registered. These should be registered outside of the example, and are
waiting for a future commit.

This removes nearly all if not all asset related functionality. Asset loading has not been implemented yet. Most notably, these changes would be required:

 - `assets::Directory` cannot be instantiated as there is no way to get an `assets::Allocator` from the `assets::Loader`.
 - Object/mesh files and texture files are waiting on PR #297

All removed code has been replaced with appropriate 'TODO's.

Additionally, this commit reformatted the example and split it up into various function, to make it easier to read. The initialisation of the lights and the camera are now fitted in their own function. The `run()` function prevents uses of `.unwrap()` and `.expect()`, and instead returns any errors to `main()` which can then print them out.
Merge pull request #2 from Binero/texture_loading
Made the assets example compile.
@Binero

This comment has been minimized.

Show comment
Hide comment
@Binero

Binero Aug 24, 2017

Contributor

There is still a few issues with this PR.

  • It exposes a lot of cgmath, for instance inside renderer::Camera.
  • It appears to me that the renderer and the asset manager are tightly linked through AssetPtr. It was decided in #190 that both systems are optional, so the asset manager must work without the renderer, and the renderer must work without the asset manager.
Contributor

Binero commented Aug 24, 2017

There is still a few issues with this PR.

  • It exposes a lot of cgmath, for instance inside renderer::Camera.
  • It appears to me that the renderer and the asset manager are tightly linked through AssetPtr. It was decided in #190 that both systems are optional, so the asset manager must work without the renderer, and the renderer must work without the asset manager.
@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Aug 24, 2017

Member

@Binero I don't know when that decision was made, but having a game without rendering doesn't make much sense to me.

Member

torkleyy commented Aug 24, 2017

@Binero I don't know when that decision was made, but having a game without rendering doesn't make much sense to me.

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Aug 24, 2017

Member

Zork! although I must say it seems unlikely a text adventure would have any need for Amethyst. :P

Member

Xaeroxe commented Aug 24, 2017

Zork! although I must say it seems unlikely a text adventure would have any need for Amethyst. :P

@Binero

This comment has been minimized.

Show comment
Hide comment
@Binero

Binero Aug 24, 2017

Contributor

@torkleyy Well, the issue is linked in the post. One of the rationales I quite like is that the components need to be reusable and replaceable. If I want to use a custom renderer, that should be possible. If I want to use a custom asset manager, that should be possible as well.

Contributor

Binero commented Aug 24, 2017

@torkleyy Well, the issue is linked in the post. One of the rationales I quite like is that the components need to be reusable and replaceable. If I want to use a custom renderer, that should be possible. If I want to use a custom asset manager, that should be possible as well.

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Aug 24, 2017

Member

The fundamental problem with that idea is that it's in direct conflict with our desire for a hot reloading feature. The two have to be coupled to make that work. The only way I can see around this would be to use cargo features to expose a different API without hot reloading capabilities when there isn't an "assets" feature present. So if modularity is truly that high of a priority for us we can do that but I feel that should be a bigger discussion with more people involved. My views personally are that we eventually have to make some architectural decisions for our end users in order to provide useful and usable software. Too many choices can hamper ease of use.

Member

Xaeroxe commented Aug 24, 2017

The fundamental problem with that idea is that it's in direct conflict with our desire for a hot reloading feature. The two have to be coupled to make that work. The only way I can see around this would be to use cargo features to expose a different API without hot reloading capabilities when there isn't an "assets" feature present. So if modularity is truly that high of a priority for us we can do that but I feel that should be a bigger discussion with more people involved. My views personally are that we eventually have to make some architectural decisions for our end users in order to provide useful and usable software. Too many choices can hamper ease of use.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Aug 24, 2017

Member

The way I see it, renderer and assets crate are separate, thus reusable. The main crate is just the glue, so if you want to e.g. use another renderer, you can still use the other crates, but you need your own glue.

Member

torkleyy commented Aug 24, 2017

The way I see it, renderer and assets crate are separate, thus reusable. The main crate is just the glue, so if you want to e.g. use another renderer, you can still use the other crates, but you need your own glue.

@Binero

This comment has been minimized.

Show comment
Hide comment
@Binero

Binero Aug 24, 2017

Contributor

One must consider that because they are so tightly coupled at the moment, it is impossible to generate meshes at runtime, as is needed in for example voxel engines or other volumetric engines.

This is because MeshComponent is an Asset, which is loaded from Loader. I think the following approach would be much more flexible:

  1. When you load a Mesh from the Loader, it gives you an AssetFuture<T> where T: Mesh which implements Component whenever T does.
  2. This AssetFuture<Mesh> can be attached to an entity.
  3. There is an AssetSystem which tracks whenever these futures have been completed, and replaces the AssetFuture<T> components on these entities with T.

As an added bonus, this system could leave behind a LoadedAsset<T> component, as to keep track of which entities use which assets. Hot reloading can then easily be implemented by simply iterating over these entities and adding new AssetFutures.

This approach has the advantage that when the asset system is not required, as it isn't for instance when you are generating meshes, it doesn't get in the way as it does now, preventing you from implementing your features.

Contributor

Binero commented Aug 24, 2017

One must consider that because they are so tightly coupled at the moment, it is impossible to generate meshes at runtime, as is needed in for example voxel engines or other volumetric engines.

This is because MeshComponent is an Asset, which is loaded from Loader. I think the following approach would be much more flexible:

  1. When you load a Mesh from the Loader, it gives you an AssetFuture<T> where T: Mesh which implements Component whenever T does.
  2. This AssetFuture<Mesh> can be attached to an entity.
  3. There is an AssetSystem which tracks whenever these futures have been completed, and replaces the AssetFuture<T> components on these entities with T.

As an added bonus, this system could leave behind a LoadedAsset<T> component, as to keep track of which entities use which assets. Hot reloading can then easily be implemented by simply iterating over these entities and adding new AssetFutures.

This approach has the advantage that when the asset system is not required, as it isn't for instance when you are generating meshes, it doesn't get in the way as it does now, preventing you from implementing your features.

Binero and others added some commits Aug 24, 2017

@omni-viral

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Aug 25, 2017

Member

Wow. Checks are passed 🎉

Member

omni-viral commented Aug 25, 2017

Wow. Checks are passed 🎉

@torkleyy torkleyy merged commit dfba2ce into develop Aug 27, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@torkleyy torkleyy deleted the renderer branch Aug 27, 2017

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