/amethyst

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

Add WindowMessages resource #387

Merged
merged 1 commit into from Oct 3, 2017

Conversation

Reviewers

@torkleyy torkleyy

@Rhuagh Rhuagh

@jojolepro jojolepro

@Aceeri Aceeri

@LucioFranco LucioFranco

@omni-viral omni-viral

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
7 participants
@Xaeroxe @Rhuagh @omni-viral @jojolepro @Aceeri @LucioFranco @torkleyy
@Xaeroxe
Member

Xaeroxe commented Oct 2, 2017

This is my take on #386

So there's a few benefits to this approach:

  • We can be optimistic and only allocate space for 2 boxes rather than being forced to be pessimistic and allocate space for 20 boxes. Thanks to SmallVec, 99.99% of the time this won't even be an extra allocation.
  • Resulting syntax is much less verbose
  • We can use the same winit::Window type rather than having the type change every time the backend changes
  • We can use FnMut instead of Fn. Technically there's no reason we couldn't use FnOnce but rustc stable doesn't support it at the moment. This is a small advantage as most of the time it won't be needed, but it's there in case my imagination has failed me.

cc @Rhuagh @jojolepro

@Xaeroxe Xaeroxe requested review from torkleyy and omni-viral Oct 2, 2017

@Xaeroxe Xaeroxe added project: rendering status: ready type: improvement labels Oct 2, 2017

@Rhuagh

Rhuagh approved these changes Oct 2, 2017
View changes

amethyst_renderer/src/renderer.rs
+ #[cfg(feature = "opengl")]
+ pub fn window(&self) -> &WinitWindow {
+ self.window.window()
+ }

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 2, 2017

Member

Newline?
@Rhuagh

Rhuagh Oct 2, 2017

Member

Newline?

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 2, 2017

Member

Added.
@Xaeroxe

Xaeroxe Oct 2, 2017

Member

Added.
@Rhuagh

Rhuagh approved these changes Oct 2, 2017
View changes

LGTM

@LucioFranco

LucioFranco approved these changes Oct 2, 2017
View changes

LGTM

@torkleyy

torkleyy reviewed Oct 2, 2017
View changes

Looks good, only a few nits. While this is not the solution I suggested, I won't vote against this solutions since the maintenance and implementation cost wouldn't be on my side. After comments addressed, r=me.

src/ecs/rendering/resources.rs
+/// renderer internal window.
+pub struct WindowCommander {
+ // It's unlikely we'll get more than two commands per frame
+ // 2 Boxes also make this the same size as a Vec, so this costs

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 2, 2017

Member

How is this true?
@torkleyy

torkleyy Oct 2, 2017

Member

How is this true?

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 2, 2017

Member

Every vector is a combination of ptr, length, and capacity. Each of these is WORD size long. When a SmallVec is inline it does not store a ptr, or capacity, it just stores the given array type and the length. Box is WORD size long so if you make the inline array type (2 * WORD size) then even the inline variant doesn't take any more space than it would if it were on the heap. Structure size isn't bloated, and you still get your smallvec optimization.
@Xaeroxe

Xaeroxe Oct 2, 2017 •
edited
Edited 1 time
  • @Xaeroxe Xaeroxe edited Oct 2, 2017 (most recent)

Member

Every vector is a combination of ptr, length, and capacity. Each of these is WORD size long. When a SmallVec is inline it does not store a ptr, or capacity, it just stores the given array type and the length. Box is WORD size long so if you make the inline array type (2 * WORD size) then even the inline variant doesn't take any more space than it would if it were on the heap. Structure size isn't bloated, and you still get your smallvec optimization.

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 2, 2017

Member

Oops, right. This comment was stupid.
@torkleyy

torkleyy Oct 2, 2017

Member

Oops, right. This comment was stupid.

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 2, 2017

Member

The two-element array somehow made me think it's two times pointer-size.
@torkleyy

torkleyy Oct 2, 2017

Member

The two-element array somehow made me think it's two times pointer-size.

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Oct 2, 2017

Member

Hmm. So the SmallVec copies data from stack to heap if you push more elements than array can hold... Is it a good choice? Why not storing only exceeding amount of elements on heap?
Anyway. Two boxed traits have size of 4 pointers though. So your structure is 5 pointers in size. While Vec is 3. 😛
@omni-viral

omni-viral Oct 2, 2017 •
edited
Edited 1 time
  • @omni-viral omni-viral edited Oct 2, 2017 (most recent)

Member

Hmm. So the SmallVec copies data from stack to heap if you push more elements than array can hold... Is it a good choice? Why not storing only exceeding amount of elements on heap?
Anyway. Two boxed traits have size of 4 pointers though. So your structure is 5 pointers in size. While Vec is 3. 😛

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 2, 2017

Member

Oh hm I guess that's true. I'll remove my comment then. As for storing what you can inline even when it spills, I had the same thought but then you can't operate on a SmallVec as if it were a slice because the memory is not contiguous.
@Xaeroxe

Xaeroxe Oct 2, 2017

Member

Oh hm I guess that's true. I'll remove my comment then. As for storing what you can inline even when it spills, I had the same thought but then you can't operate on a SmallVec as if it were a slice because the memory is not contiguous.
src/ecs/rendering/resources.rs
+ // 2 Boxes also make this the same size as a Vec, so this costs
+ // no more space in the structure than a Vec would.
+ //
+ // NOTE TO FUTURE AUTHORS: This could be an FnOnce but that's not possible

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 2, 2017

Member

I solved this in https://github.com/slide-rs/specs/pull/214/files , maybe you can use that.
@torkleyy

torkleyy Oct 2, 2017

Member

I solved this in https://github.com/slide-rs/specs/pull/214/files , maybe you can use that.

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 2, 2017

Member

Ha! Not bad! I like it. I'll give it a shot.
@Xaeroxe

Xaeroxe Oct 2, 2017

Member

Ha! Not bad! I like it. I'll give it a shot.
src/ecs/rendering/resources.rs
+ }
+ }
+
+ /// Execute this closure on the winit::Window next frame.

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 2, 2017

Member

Missing backticks
@torkleyy

torkleyy Oct 2, 2017

Member

Missing backticks
src/ecs/rendering/resources.rs
+}
+
+impl WindowCommander {
+ /// Create a new WindowCommander

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 2, 2017

Member

nit: missing backticks
@torkleyy

torkleyy Oct 2, 2017

Member

nit: missing backticks
src/ecs/rendering/resources.rs
+ pub(crate) queue: SmallVec<[Box<FnMut(&Window) + Send + Sync + 'static>; 2]>,
+}
+
+impl WindowCommander {

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 2, 2017

Member

Meh, this name sounds more like a strategy RTS game's name. Could we rename it? Maybe WindowMessages?
@torkleyy

torkleyy Oct 2, 2017

Member

Meh, this name sounds more like a strategy RTS game's name. Could we rename it? Maybe WindowMessages?

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 2, 2017

Member

Hahaha 😆 "Coming soon to stores near you! Window Commander!! Tell panes of glass to do things which they'll proceed to ignore because they're just glass! Copyright 2017"

I'll rename it :P
@Xaeroxe

Xaeroxe Oct 2, 2017

Member

Hahaha 😆 "Coming soon to stores near you! Window Commander!! Tell panes of glass to do things which they'll proceed to ignore because they're just glass! Copyright 2017"

I'll rename it :P
@jojolepro

jojolepro approved these changes Oct 2, 2017
View changes

Not a big fan of having two types of message passing methods, but I guess its fine.

@Aceeri

Aceeri approved these changes Oct 2, 2017
View changes

Looks good to me, just address torkleyys comments.

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 3, 2017

Member

bors r=@torkleyy
Member

Xaeroxe commented Oct 3, 2017

bors r=@torkleyy

@Xaeroxe Xaeroxe dismissed torkleyy’s stale review Oct 3, 2017

Addressed

bors bot added a commit that referenced this pull request Oct 3, 2017

@bors[bot]
Merge #387 
387: Add WindowCommander resource r=torkleyy a=Xaeroxe

This is my take on #386

So there's a few benefits to this approach:

- We can be optimistic and only allocate space for 2 boxes rather than being forced to be pessimistic and allocate space for 20 boxes.  Thanks to SmallVec, 99.99% of the time this won't even be an extra allocation.
- Resulting syntax is much less verbose
- We can use the same winit::Window type rather than having the type change every time the backend changes
- We can use FnMut instead of Fn.  Technically there's no reason we couldn't use FnOnce but rustc stable doesn't support it at the moment.  This is a small advantage as most of the time it won't be needed, but it's there in case my imagination has failed me.

cc @Rhuagh @jojolepro
Loading status checks…
ab60055

@Xaeroxe Xaeroxe changed the title from Add WindowCommander resource to Add WindowMessages resource Oct 3, 2017

@Xaeroxe
Add WindowMessages resource
Loading status checks…
8d8c121
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot Oct 3, 2017

Contributor

Canceled
Contributor

bors bot commented Oct 3, 2017

Canceled
@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 3, 2017

Member

bors r=@torkleyy

Had to fix my commit message real quick.
Member

Xaeroxe commented Oct 3, 2017

bors r=@torkleyy

Had to fix my commit message real quick.

bors bot added a commit that referenced this pull request Oct 3, 2017

@bors[bot]
Merge #387 
387: Add WindowMessages resource r=torkleyy a=Xaeroxe

This is my take on #386

So there's a few benefits to this approach:

- We can be optimistic and only allocate space for 2 boxes rather than being forced to be pessimistic and allocate space for 20 boxes.  Thanks to SmallVec, 99.99% of the time this won't even be an extra allocation.
- Resulting syntax is much less verbose
- We can use the same winit::Window type rather than having the type change every time the backend changes
- We can use FnMut instead of Fn.  Technically there's no reason we couldn't use FnOnce but rustc stable doesn't support it at the moment.  This is a small advantage as most of the time it won't be needed, but it's there in case my imagination has failed me.

cc @Rhuagh @jojolepro
Loading status checks…
48684d9
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot Oct 3, 2017

Contributor

Build succeeded
Contributor

bors bot commented Oct 3, 2017

Build succeeded

@bors bors bot merged commit 8d8c121 into amethyst:develop Oct 3, 2017
3 checks passed

3 checks passed

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

@Xaeroxe Xaeroxe deleted the Xaeroxe:window_command branch Oct 7, 2017

@Xaeroxe Xaeroxe referenced this pull request Oct 12, 2017

Closed

Extend window functionality #162

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