Tracked #311

Aceeri · 2017-12-07T09:16:27Z

Somehow I managed to break github with that last PR... and now it won't let me re-open it so here's a new one.

Old PR: #305

This change is

Aceeri · 2017-12-07T09:57:36Z

To address reviewable comments (won't let me put them on the old reviewable :/)

This should use match and return the reader id.

Fixed this

Should this use the restricted storage?

Yep, since that is probably better than the cloning of the bitset.

I don't agree with this strategy, but don't have a constructive suggestion at the moment.

I just wanted it just in case someone wanted a higher buffer capacity for a component. It shouldn't effect usability since it just defaults out.

Do we really need to export this?

Maybe, not entirely sure. I was thinking maybe we should have it open ended so someone could also use this trait to populate other structures with events.

torkleyy · 2017-12-07T17:29:22Z

I just wanted it just in case someone wanted a higher buffer capacity for a component. It shouldn't effect usability since it just defaults out.

The problem I see with buffer overflows is that there will be things that depend on accuracy, e.g. a separate list of the components. If the insertions / deletions overflow, this could lead to serious bugs.

torkleyy · 2017-12-13T15:29:21Z

Is this ready for review?

Review status: 0 of 11 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

Aceeri · 2017-12-13T19:40:28Z

@torkleyy Yep

torkleyy · 2017-12-14T13:59:20Z

Reviewed 5 of 11 files at r1.
Review status: 5 of 11 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

src/lib.rs, line 219 at r1 (raw file):

pub use shred::AsyncDispatcher;

pub use storage::{BTreeStorage, Capacity, DefaultCapacity, DenseVecStorage, DistinctStorage,

We really need some organization here!

Comments from Reviewable

Aceeri · 2017-12-16T07:07:46Z

src/lib.rs, line 219 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

We really need some organization here!

Should we maybe separate them into multiple pub use ... statements?

Comments from Reviewable

torkleyy · 2017-12-16T07:23:24Z

Probably not do unnecessary exports at all. Like keep most stuff in the modules and define a prelude module. The storage module got too big, it should probably be split up.

Aceeri · 2017-12-18T10:31:08Z

src/lib.rs, line 219 at r1 (raw file):

Previously, Aceeri (Aceeri) wrote…

Should we maybe separate them into multiple pub use ... statements?

Added a prelude module for the stuff and made the modules public instead. We still might want to split up storage though.

Comments from Reviewable

torkleyy · 2017-12-19T14:39:11Z

Reviewed 1 of 11 files at r1, 40 of 40 files at r3.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

Cargo.toml, line 25 at r3 (raw file):

fnv = "1.0"
#hibitset = "0.3.2"
hibitset = { git = "https://github.com/slide-rs/hibitset", commit = "524e075728babcd4297248c3e17771df437022d4", version = "0.3.2" }

We'll need to change this once I released a new version; I'd like to do that before merging this PR, but also after merging the hibitset PRs.

benches/parallel.rs, line 11 at r3 (raw file):

use rand::thread_rng;
use specs::{Component, DenseVecStorage, DispatcherBuilder, Entities, Entity, Fetch,
            HashMapStorage, Join, NullStorage, ReadStorage, RunningTime, System, VecStorage,

RunningTime should certainly not be in the prelude. Same is probably true for NullStorage and HashMapStorage.

src/error.rs, line 64 at r3 (raw file):

    #[doc(hidden)]
    __NonExhaustive,

Please don't change this.

src/lib.rs, line 219 at r1 (raw file):

Previously, Aceeri (Aceeri) wrote…

Added a prelude module for the stuff and made the modules public instead. We still might want to split up storage though.

Thanks for changing the module structure; would prefer if you could split it up into two PRs next time.

src/prelude.rs, line 7 at r3 (raw file):

pub use hibitset::BitSet;
pub use join::{Join, ParJoin};
pub use shred::{Dispatcher, DispatcherBuilder, Fetch, FetchId, FetchIdMut, FetchMut, RunNow,

FetchId and FetchIdMut can be removed, nobody needs them^^

src/prelude.rs, line 8 at r3 (raw file):

pub use join::{Join, ParJoin};
pub use shred::{Dispatcher, DispatcherBuilder, Fetch, FetchId, FetchIdMut, FetchMut, RunNow,
                RunningTime, System, SystemData};

As I've said, RunningTime does not qualify as common structure. I'm pretty sure most people don't even know about it.

src/prelude.rs, line 14 at r3 (raw file):

pub use shred::AsyncDispatcher;

pub use storage::{DenseVecStorage, FlaggedStorage, HashMapStorage, InsertedFlag, ModifiedFlag,

I would remove the flags, the NullStorage and the HashMapStorage, too.

src/prelude.rs, line 17 at r3 (raw file):

                  NullStorage, ReadStorage, RemovedFlag, Storage, Tracked, VecStorage,
                  WriteStorage};
pub use world::{Component, Entities, EntitiesRes, Entity, EntityBuilder, Generation, LazyUpdate,

Generation and EntitiesRes can be removed, too.

src/storage/flagged.rs, line 23 at r3 (raw file):

/// ```rust
/// extern crate specs;
/// extern crate shrev;

Still necessary?

src/storage/flagged.rs, line 131 at r3 (raw file):

    unsafe fn remove(&mut self, id: Index) -> C {
        self.removed.single_write(Flag::Flag(id).into());

Err, so when would you emit an Unflag event?

src/storage/mod.rs, line 12 at r3 (raw file):

pub use self::storages::RudyStorage;
pub use self::track::{Flag, InsertedFlag, ModifiedFlag, RemovedFlag, Tracked};

We gotta split this module (later, not in this PR).

src/storage/track.rs, line 69 at r3 (raw file):

        E: Extend<Flag>,
    {
        value.extend(self.removed().read(reader_id).map(|flag| *flag.as_ref()));

Nice 👍

src/world/entity.rs, line 46 at r3 (raw file):

/// Internally used structure for `Entity` allocation.
#[derive(Default, Debug)]
pub struct Allocator {

Please don't expose this, make it pub(crate).

Comments from Reviewable

torkleyy · 2017-12-19T14:39:37Z

Wow, this was a lot to review.

Aceeri · 2017-12-20T05:29:41Z

src/error.rs, line 64 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Please don't change this.

Looks like rustfmt nightly decided to do this.

Comments from Reviewable

Aceeri · 2017-12-20T05:30:29Z

src/lib.rs, line 219 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Thanks for changing the module structure; would prefer if you could split it up into two PRs next time.

Sorry about that, originally started working on it on this branch, then realized a bit late.

Comments from Reviewable

torkleyy · 2017-12-20T12:28:24Z

Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

src/world/entity.rs, line 46 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Please don't expose this, make it pub(crate).

OK, thanks for resolving.

Comments from Reviewable

torkleyy · 2017-12-25T21:52:24Z

Great, thanks! Some more docs would be nice, yeah.

Reviewed 22 of 22 files at r5.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

src/error.rs, line 64 at r3 (raw file):

Previously, Aceeri (Aceeri) wrote…

Looks like rustfmt nightly decided to do this.

Thanks for fixing!

Comments from Reviewable

torkleyy · 2017-12-27T06:52:11Z

Reviewed 10 of 10 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

src/storage/flagged.rs, line 55 at r6 (raw file):

///         // can only clear it when necessary. (This might be useful if you have some
///         // sort of "tick" system in your game and only want to do operations every
///         // 1/4th of a second or something)

Maybe we should add here that it's not okay to just read the events in some interval because it would make shrev's buffer allocation go crazy.

Comments from Reviewable

Aceeri · 2017-12-30T07:03:51Z

src/storage/flagged.rs, line 131 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Err, so when would you emit an Unflag event?

Removed it, was remnants of the Tracked/ChangedStorage

Comments from Reviewable

Aceeri · 2017-12-30T07:04:01Z

src/storage/flagged.rs, line 55 at r6 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Maybe we should add here that it's not okay to just read the events in some interval because it would make shrev's buffer allocation go crazy.

Done.

Comments from Reviewable

Aceeri · 2017-12-30T07:04:41Z

src/storage/flagged.rs, line 23 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Still necessary?

Done.

Comments from Reviewable

torkleyy · 2017-12-30T07:08:04Z

Looks good to me, but there are some conflicts you need to resolve. And please don't publish every comment separately next time :)

Reviewed 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

Aceeri · 2017-12-30T08:04:11Z

@torkleyy Sorry, somewhat confused by the reviewable ui.

kvark · 2018-01-02T03:13:15Z

Phew! looks like a solid step forward, although I reserve a few concerns ;)

Reviewed 3 of 11 files at r1, 16 of 40 files at r3, 13 of 22 files at r5, 8 of 10 files at r6, 1 of 1 files at r7, 3 of 3 files at r8.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

CHANGELOG.md, line 3 at r8 (raw file):

## [Unreleased]

* Remove `FlaggedStorage` (new storage uses the same name) and `TrackedStorage` in favor of `Tracked` api. ([#305])

let's have "in favor of the new Tracked API"

specs-derive/src/lib.rs, line 62 at r8 (raw file):

    quote! {
        impl #impl_generics ::specs::world::Component for #name #ty_generics #where_clause {

isn't this a breaking change for this crate?

src/common.rs, line 257 at r8 (raw file):

    use shred::DispatcherBuilder;
    use storage::{NullStorage, VecStorage};
    use world::{Component, World};

I like the new structure 👍

src/join.rs, line 13 at r8 (raw file):

/// `BitAnd` is a helper method to & bitsets together resulting in a tree.
pub trait BitAnd {
    /// The combined bitsets.

oh, bonus points for the docs!

src/storage/flagged.rs, line 128 at r8 (raw file):

/// ```
pub struct FlaggedStorage<C, T = DenseVecStorage<C>> {
    modified: EventChannel<ModifiedFlag>,

Have you described somewhere the pros/cons of using events for flagging as opposed to the old bit set? It's not obvious to me that this is going to be an improvement perf-wise and complexity-wise.

src/storage/track.rs, line 14 at r8 (raw file):

pub trait Tracked {
    /// Event channel tracking modified components.
    fn modified(&self) -> &EventChannel<ModifiedFlag>;

if the owner is supposed to just have those three channels for borrows, can we declare a struct with them and just expose this struct in Tracked trait API?
e.g.

pub struct TrackChannels {
  pub modify: EventChannel<ModifiedFlag>,
  pub insert: EventChannel<InsertedFlag>,
  pub remove: EventChannel<RemovedFlag>,
}

Comments from Reviewable

Aceeri · 2018-01-02T04:33:11Z

specs-derive/src/lib.rs, line 62 at r8 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

isn't this a breaking change for this crate?

Well, internally it is, to anyone using it is it actually less breaking than if they implemented Component themselves.

Comments from Reviewable

torkleyy · 2018-01-02T04:48:11Z

Well we're changing the crate structure here, how could it not be breaking?

kvark · 2018-01-02T05:04:23Z

So the version should become 0.2.0

…

On Jan 1, 2018, at 23:48, Thomas Schaller ***@***.***> wrote: Well we're changing the crate structure here, how could it not be breaking? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

Aceeri · 2018-01-02T05:49:17Z

I just mean that specs-derive isn't breaking (all the specs implementation stuff is internal for it, none of it is exposed to the user), specs itself is.

torkleyy · 2018-01-02T06:54:30Z

Err but users of 0.10 will get the latest version of specs-derive once they update. That in turn will break their code.

torkleyy · 2018-01-07T09:57:05Z

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

src/storage/flagged.rs, line 128 at r8 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Have you described somewhere the pros/cons of using events for flagging as opposed to the old bit set? It's not obvious to me that this is going to be an improvement perf-wise and complexity-wise.

The main motivation for this is that it's logically necessary to use events, otherwise you would miss events. Given two readers, A and B, imagine the following steps:

A reads
B reads
clear events

If B makes an changes, they will be cleared before A can read them.

Comments from Reviewable

Aceeri · 2018-01-26T03:22:27Z

Review status: 39 of 44 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

CHANGELOG.md, line 3 at r8 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

let's have "in favor of the new Tracked API"

Done.

specs-derive/src/lib.rs, line 62 at r8 (raw file):

Previously, Aceeri (Aceeri) wrote…

Well, internally it is, to anyone using it is it actually less breaking than if they implemented Component themselves.

Never mind, I was being stupid about this. Incremented the minor version for it.

src/storage/track.rs, line 14 at r8 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

if the owner is supposed to just have those three channels for borrows, can we declare a struct with them and just expose this struct in Tracked trait API?
e.g.
pub struct TrackChannels {
  pub modify: EventChannel<ModifiedFlag>,
  pub insert: EventChannel<InsertedFlag>,
  pub remove: EventChannel<RemovedFlag>,
}

Good idea! :)

Comments from Reviewable

Aceeri · 2018-01-26T05:44:22Z

Review status: 33 of 44 files reviewed at latest revision, 6 unresolved discussions.

Cargo.toml, line 25 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

We'll need to change this once I released a new version; I'd like to do that before merging this PR, but also after merging the hibitset PRs.

Done.

Comments from Reviewable

torkleyy · 2018-01-31T17:11:51Z

Please review again @kvark

Reviewed 4 of 8 files at r9, 7 of 7 files at r10.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

kvark · 2018-01-31T17:44:55Z

some commits can probably be squashed, leaving for your discretion

Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

bors · 2018-01-31T17:55:26Z

✌️ Aceeri can now approve this pull request

torkleyy

Please squash commits and merge, @Aceeri

bors delegate+

…, formatting, etc.

…ReaderIds, etc.

…ersion

Aceeri · 2018-02-08T06:46:28Z

bors r+

311: Tracked r=Aceeri a=Aceeri Somehow I managed to break github with that last PR... and now it won't let me re-open it so here's a new one. Old PR: #305  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/slide-rs/specs/311)

bors · 2018-02-08T07:08:19Z

Build succeeded

continuous-integration/travis-ci/push

Aceeri force-pushed the interface branch 2 times, most recently from ccf85c1 to 9e40d6a Compare December 18, 2017 06:33

Aceeri force-pushed the interface branch 3 times, most recently from e5be25b to 045b031 Compare December 23, 2017 20:04

Aceeri force-pushed the interface branch from 48e5cb5 to 06b1f1e Compare December 26, 2017 01:57

torkleyy added the ready label Dec 30, 2017

torkleyy requested a review from kvark December 30, 2017 07:08

Aceeri force-pushed the interface branch from 199f454 to 65fea28 Compare December 30, 2017 20:57

torkleyy mentioned this pull request Jan 7, 2018

Get entities sorted by component value #333

Closed

Aceeri force-pushed the interface branch 2 times, most recently from ccbf268 to 26d537f Compare January 26, 2018 05:42

Aceeri force-pushed the interface branch from 26d537f to 55f7c93 Compare January 26, 2018 05:48

kvark removed their request for review January 31, 2018 17:45

torkleyy approved these changes Jan 31, 2018

View reviewed changes

Aceeri force-pushed the interface branch from 55f7c93 to de1b875 Compare February 8, 2018 05:33

Aceeri added 4 commits February 7, 2018 21:34

Tracked trait and methods

8ca72a5

Prelude module, remove Capacity, use Extend<Flag> instead of Populate…

bf27711

…, formatting, etc.

Better documentation, explain usage over populating bitsets, getting …

a97aefe

…ReaderIds, etc.

Move event channels into a dedicated struct, increment derive crate v…

593aa98

…ersion

Aceeri force-pushed the interface branch from de1b875 to 593aa98 Compare February 8, 2018 05:34

bors bot merged commit 593aa98 into amethyst:master Feb 8, 2018

Tracked #311

Tracked #311

Conversation

Aceeri commented Dec 7, 2017 • edited by torkleyy Loading

Aceeri commented Dec 7, 2017

torkleyy commented Dec 7, 2017

torkleyy commented Dec 13, 2017

Aceeri commented Dec 13, 2017

torkleyy commented Dec 14, 2017

Aceeri commented Dec 16, 2017

torkleyy commented Dec 16, 2017

Aceeri commented Dec 18, 2017

torkleyy commented Dec 19, 2017

torkleyy commented Dec 19, 2017

Aceeri commented Dec 20, 2017

Aceeri commented Dec 20, 2017

torkleyy commented Dec 20, 2017

torkleyy commented Dec 25, 2017

torkleyy commented Dec 27, 2017

Aceeri commented Dec 30, 2017

Aceeri commented Dec 30, 2017

Aceeri commented Dec 30, 2017

torkleyy commented Dec 30, 2017

Aceeri commented Dec 30, 2017

kvark commented Jan 2, 2018

Aceeri commented Jan 2, 2018

torkleyy commented Jan 2, 2018

kvark commented Jan 2, 2018 via email

Aceeri commented Jan 2, 2018 • edited Loading

torkleyy commented Jan 2, 2018 • edited Loading

torkleyy commented Jan 7, 2018

Aceeri commented Jan 26, 2018

Aceeri commented Jan 26, 2018

torkleyy commented Jan 31, 2018

kvark commented Jan 31, 2018

bors bot commented Jan 31, 2018

torkleyy left a comment

Choose a reason for hiding this comment

Aceeri commented Feb 8, 2018

bors bot commented Feb 8, 2018

Build succeeded

Aceeri commented Dec 7, 2017 •

edited by torkleyy

Loading

Aceeri commented Jan 2, 2018 •

edited

Loading

torkleyy commented Jan 2, 2018 •

edited

Loading