-
-
Notifications
You must be signed in to change notification settings - Fork 743
Conversation
I'll need a more complex example to really get the feel of it. The prototype looks good ;) |
The World is not replaced, it's just the Dispatcher that's been replaced with a generic type. |
I was thinking of maybe adding a loading state to the renderable example, since some of those assets take a while to load in debug mode. |
This is basically my stab at #576. Because we use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be a good idea to add a chapter to the book called Advanced Game Architecture that shows how to use multiple dispatchers in a custom |
I was planning on creating an example atleast once I get some feedback on the basic design :) |
I'll re-review it soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, thanks for your work! A chapter would really be helpful, I'm not sure I understand how all the *Data
types interact exactly.
amethyst_animation/src/util.rs
Outdated
controls.entry(entity).unwrap().or_insert_with(AnimationControlSet::default) | ||
controls | ||
.entry(entity) | ||
.unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not unwrap but return the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a known (and reported issue), yes, will fix that in a separate PR as it changes the API.
@@ -0,0 +1,240 @@ | |||
use std::sync::Arc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing top-level docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good, but since this is a really big breaking change, I have a few questions.
Main one:
Is it possible to have a GameData that has:
global_dispatch
state_specific_dispatch
where the state_specific_dispatch is created during a State start() method?
Basically, sometimes you want to have systems run only for some states (like in this example), but you want the state to conditionally set the systems. Also, creating it in the start (or similar) method makes it easy to find the relation between State and state_specific_dispatch.
I don't want to have
GameData
-global_dispatch
-state1_dispatch
-state2_dispatch
-state3_dispatch
-state4_dispatch
Especially since sometimes creating a System might need data loaded by a previous state. For example, a System specific to GameMode1State would require a Handle loaded by LoadingState.
amethyst_ui/src/layout.rs
Outdated
(entities, mut locals, parents, anchors, stretches, screen_dim, hierarchy): Self::SystemData, | ||
){ | ||
fn run(&mut self, data: Self::SystemData) { | ||
let (entities, mut locals, parents, anchors, stretches, screen_dim, hierarchy) = data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting. The previous code complained about too long row every time I ran rustfmt, and I got tired of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
examples/game_data/README.md
Outdated
@@ -0,0 +1,5 @@ | |||
# Renderable Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Renderable/Game Data/
examples/game_data/game_data.rs
Outdated
use amethyst::{DataInit, Error, Result}; | ||
use rayon::ThreadPool; | ||
|
||
pub struct GameData<'a, 'b> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got really really confused, because this custom GameData has the same name has amethyst::GameData. I couldn't figure out why this one had two dispatchers and the other one had one.
Please rename it to CustomGameData.
examples/game_data/main.rs
Outdated
@@ -0,0 +1,583 @@ | |||
//! Demonstrates how to load renderable objects, along with several lighting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt it!
examples/game_data/main.rs
Outdated
//! Demonstrates how to load renderable objects, along with several lighting | ||
//! methods. | ||
//! | ||
//! TODO: Rewrite for new renderer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmmmmmmm :P
examples/game_data/main.rs
Outdated
|
||
mod game_data; | ||
|
||
struct DemoState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having all the lighting stuff makes the example way more complicated than how it needs to be. Its harder to really spot what GameData does when there is so much unrelated code. A simple rotating cube with a fixed camera is all you would need I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The renderable example is the only one where loading is slow enough to actually warrant a Loading state, that's why I chose it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could keep the .obj loading, which is the slow part, but remove the lights. If not then its fine.
examples/game_data/main.rs
Outdated
data.data.update(&data.world, true); | ||
match self.progress.as_ref().unwrap().complete() { | ||
Completion::Failed => { | ||
println!("Failed loading assets"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
printerr! or error!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't see a need to pull in an extra crate for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
printerr! then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
printerr? Not a part of core rust from what I know ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya sorry its "eprintln!" actually :P
examples/game_data/main.rs
Outdated
.with_pass(DrawShaded::<PosNormTex>::new()) | ||
.with_pass(DrawUi::new()), | ||
); | ||
let game_data = GameDataBuilder::default() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Games don't only have a base and a running state. There should be a way to provide multiple systems that are specific to each of the states. Those systems should be added during the state start function, to allow for better logic locality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ofc not, but I'm not about to write a whole game in a single example, and this example is for demonstrating a custom game data implementation, not teach people how to structure a game code wise.
( | ||
fullscreen: false, | ||
multisampling: 0, | ||
title: "Renderable Example", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renderable again
It's a generic type without bounds, you can put anything you like in it. Data would be in the |
So GameData is global? If so I'm not sure what you can do with it that you couldn't do using Resource. |
@Jojolepro The main thrust of the PR is to permit people to access data in a way that isn't via |
|
@Rhuagh mentioned #576 in a comment on this PR (Maybe we should mention it in the opening PR description) If we go to that issue some helpful context is present. Specifically this quote:
|
And how does Resource not solve this? My current plan for a voxel world involves having it as a resource and having Systems use that resource to create entities (optimised) that will end up getting rendered. For me, this change is replicating what specs Resources is doing already. |
Yeah a |
You can't store systems in a resource. |
@Rhuagh is correct due to this: https://docs.rs/shred/0.7.0-alpha5/shred/struct.Dispatcher.html#impl-Send |
Re-review if you can please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not a fan of all the lighting stuff, but that's fine ;)
Thanks!
Didn't we have that discussion before? |
Reviewed 3 of 8 files at r1, 5 of 10 files at r2, 19 of 21 files at r3, 3 of 3 files at r4, 1 of 2 files at r6, 13 of 20 files at r7. Comments from Reviewable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I thought cfg!(target_os = "ios") didn't work? |
I still need to add a chapter to the book, and I'm currently focusing on getting specs out. |
Rebased, squashed, book chapter added. @Xaeroxe feel free to take over. |
Ready for final review @Jojolepro @torkleyy @Xaeroxe |
outside most of the day, I'll review tonight if I can, or tomorow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! bors r+
err actually nevermind, I think this might fail on CI due to some new examples. |
36cfca2
to
6d3f65a
Compare
bors r=Xaeroxe,torkleyy,jojolepro |
691: refactor: Make game data pluggable r=Xaeroxe,torkleyy,jojolepro a=Rhuagh Remove Dispatcher from Application, and add a pluggable generic type. Add a direct replacement for the old game data, featuring only a Dispatcher. Make the states take a new StateData type, containing references to World and pluggable game data. Move responsibility for doing system dispatch to the states. Opening this early to get feedback. Resolves #576 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/amethyst/amethyst/691) <!-- Reviewable:end --> Co-authored-by: Simon Rönnberg <seamonr@gmail.com>
Build failed |
bors r=Xaeroxe,torkleyy,jojolepro |
691: refactor: Make game data pluggable r=Xaeroxe,torkleyy,jojolepro a=Rhuagh Remove Dispatcher from Application, and add a pluggable generic type. Add a direct replacement for the old game data, featuring only a Dispatcher. Make the states take a new StateData type, containing references to World and pluggable game data. Move responsibility for doing system dispatch to the states. Opening this early to get feedback. Resolves #576 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/amethyst/amethyst/691) <!-- Reviewable:end --> Co-authored-by: Simon Rönnberg <seamonr@gmail.com>
Remove Dispatcher from Application, and add a pluggable generic type.
Add a direct replacement for the old game data, featuring only a Dispatcher.
Make the states take a new StateData type, containing references to World and pluggable game data.
Move responsibility for doing system dispatch to the states.
Opening this early to get feedback.
Resolves #576
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)