Add amethyst_context crate #63

nchashch · Jun 23, 2016

Add Context struct which owns all global resources e.g. Window, RenderingContext, AudioContext, Logger, AssetManager, NetworkManager, etc.

Context is created from Config (it is an amethyst_config sturct which holds subconfigs for particular resources) and Rc<RefCell<Context>> is passed to every party requiring access to the context (see examples/window.rs).

Currently Context only contains a Window and a RenderingContext (struct containing device, factory, render targets and an amethyst_renderer::Renderer).

Potentially it can be used for event handling (passing an Rc<RefCell<Context>> to Application and polling events from it in Applcation::advance_frame).

It should close #54

Looks good to me. Summoning @ebkalderon , @csherratt , @Aceeri and whoever else interested!

kvark · Jun 23, 2016

Looks good to me. Summoning @ebkalderon , @csherratt , @Aceeri and whoever else interested!

palodequeso · Jun 23, 2016

palodequeso reviewed Jun 23, 2016

View changes

palodequeso · Jun 23, 2016

src/context/src/rendering_context.rs

+            .with_visibility(visibility);
+
+        match display_config.min_dimensions {
+            Some ((w, h)) => builder = builder.with_dimensions(w, h),


Why not combine line 56's some block into this one?

It is an error, on line 51 display_config.min_dimensions should be display_config.dimensions, thanks for pointing it out!

Wee, I was useful!

I had one small nit-pick, but I still consider myself a rust beginner, so there may be a really good reason for not doing what I suggested.

palodequeso · Jun 23, 2016

I had one small nit-pick, but I still consider myself a rust beginner, so there may be a really good reason for not doing what I suggested.

+1

palodequeso · Jun 23, 2016

+1

Seems good, but should be kept in mind that there are other than GL contexts :)

White-Oak · Jun 23, 2016

Seems good, but should be kept in mind that there are other than GL contexts :)

Seems good, but should be kept in mind that there are other than GL contexts :)

Agreed.

palodequeso · Jun 24, 2016

Seems good, but should be kept in mind that there are other than GL contexts :)

Agreed.

Looks good to me, the GL context problem should probably be handled separately in the amethyst_renderer crate with some sort of Window trait over glutin/glfw/sdl/dx11/metal stuff.

Aceeri · Jun 25, 2016

Looks good to me, the GL context problem should probably be handled separately in the amethyst_renderer crate with some sort of Window trait over glutin/glfw/sdl/dx11/metal stuff.

Aceeri · Jun 25, 2016

Aceeri reviewed Jun 25, 2016

View changes

Aceeri · Jun 25, 2016

src/context/src/rendering_context.rs

+            .with_multisampling(multisampling)
+            .with_visibility(visibility);
+
+        match display_config.dimensions {


Since the None branch never actually does something, I think it might be better to just make this a

if let Some((w, h)) = display_config.dimensions { builder = builder.with_dimensions(w, h); }

Aceeri · Jun 26, 2016

Aceeri reviewed Jun 26, 2016

View changes

Aceeri · Jun 26, 2016

src/context/src/rendering_context.rs

+    struct DisplayConfig {
+        pub title: String = "Amethyst game".to_string(),
+        pub fullscreen: bool = false,
+        pub dimensions: Option<(u32, u32)> = None,


May want to change this to just (u32, u32) instead of an Option, since otherwise it would be defaulted to None and the window won't know what the default resolution is.

I think it should be an Option for ergonomics reasons. If display_config.dimensions is an Option then it is possible to get a Window in fullscreen with native resolution just by passing null to display_config.dimensions field, and if it is not in fullscreen then it will just default to some arbituary dimensions.

If display_config.dimensions is not an Option then in fullscreen resolution would be set to some hard coded value that doesn't necessarily match the native resolution in case if the proper resolution is not set in the config. Which might be unexpected.

True, forgot that setting dimensions when in exclusive mode might create some problems.

Looks like everyone is happy. Merging.

kvark · Jul 1, 2016

Looks like everyone is happy. Merging.

nchashch added some commits Jun 23, 2016

Add amethyst_context crate
Allows windowing support

c1848e7

Add newline to config/window_example_config.yml
To prevent git from complaining

1965b60

Small fix in amethyst_context

38ff2e5

Minor refactoring

418f352

kvark merged commit df9e449 into amethyst:develop Jul 1, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

nchashch deleted the nchashch:windowing branch Jul 12, 2016

nchashch referenced this pull request Jul 16, 2016
Closed
Windowing Support #54

amethyst/amethyst

Add amethyst_context crate #63

nchashch commented Jun 23, 2016 •

edited

Edited 1 time

nchashch edited Jun 23, 2016 (most recent)

nchashch added some commits Jun 23, 2016

This comment has been minimized.

kvark commented Jun 23, 2016

palodequeso reviewed Jun 23, 2016

View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

palodequeso commented Jun 23, 2016

This comment has been minimized.

palodequeso commented Jun 23, 2016

This comment has been minimized.

White-Oak commented Jun 23, 2016

This comment has been minimized.

palodequeso commented Jun 24, 2016

This comment has been minimized.

Aceeri commented Jun 25, 2016

Aceeri reviewed Jun 25, 2016

View changes

This comment has been minimized.

Aceeri reviewed Jun 26, 2016

View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kvark commented Jul 1, 2016

kvark merged commit `df9e449` into amethyst:develop Jul 1, 2016
1 check passed

1 check passed

nchashch deleted the nchashch:windowing branch Jul 12, 2016

nchashch referenced this pull request Jul 16, 2016

Windowing Support #54

amethyst/amethyst

Join GitHub today

Add amethyst_context crate #63

Conversation

nchashch commented Jun 23, 2016 • edited Edited 1 time nchashch edited Jun 23, 2016 (most recent)

nchashch added some commits Jun 23, 2016

This comment has been minimized.

kvark commented Jun 23, 2016

palodequeso reviewed Jun 23, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

palodequeso commented Jun 23, 2016

This comment has been minimized.

palodequeso commented Jun 23, 2016

This comment has been minimized.

White-Oak commented Jun 23, 2016

This comment has been minimized.

palodequeso commented Jun 24, 2016

This comment has been minimized.

Aceeri commented Jun 25, 2016

Aceeri reviewed Jun 25, 2016 View changes

This comment has been minimized.

Aceeri Jun 25, 2016 • edited Edited 1 time Aceeri edited Jun 25, 2016 (most recent)

Aceeri reviewed Jun 26, 2016 View changes

This comment has been minimized.

This comment has been minimized.

nchashch Jun 26, 2016 • edited Edited 1 time nchashch edited Jun 26, 2016 (most recent)

This comment has been minimized.

This comment has been minimized.

kvark commented Jul 1, 2016

Hide details View details kvark merged commit df9e449 into amethyst:develop Jul 1, 2016 1 check passed

1 check passed

nchashch deleted the nchashch:windowing branch Jul 12, 2016

nchashch referenced this pull request Jul 16, 2016

nchashch commented Jun 23, 2016 •

edited

Edited 1 time

nchashch edited Jun 23, 2016 (most recent)

palodequeso reviewed Jun 23, 2016

View changes

Aceeri reviewed Jun 25, 2016

View changes

Aceeri Jun 25, 2016 •

edited

Edited 1 time

Aceeri edited Jun 25, 2016 (most recent)

Aceeri reviewed Jun 26, 2016

View changes

nchashch Jun 26, 2016 •

edited

Edited 1 time

nchashch edited Jun 26, 2016 (most recent)

kvark merged commit `df9e449` into amethyst:develop Jul 1, 2016
1 check passed