Skip to content
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

Updated SPECS #1923

Merged
merged 4 commits into from Sep 20, 2019

Conversation

@AndreaCatania
Copy link
Contributor

commented Sep 9, 2019

Description

Updated SPECS to the last version and using the new flagged storage feature

PR Checklist

  • Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme.

If this modified or created any rs files:

  • Ran cargo +stable fmt --all
  • Ran cargo clippy --all --features "empty"
  • Ran cargo test --all --features "empty"
@@ -25,8 +25,8 @@ log = "0.4.6"
num-traits = "0.2.0"
rayon = "1.1.0"
serde = { version = "1", features = ["derive"] }
specs = { version = "0.15", features = ["shred-derive"] }
specs-hierarchy = "0.5.1"
specs = { version = "0.16", features = ["shred-derive", "storage-event-control"] }

This comment has been minimized.

Copy link
@azriel91

azriel91 Sep 11, 2019

Member

hm, could you enable the "specs-derive" feature by default on specs, and for "storage-event-control", have that as a feature of amethyst and amethyst_core, which, when enabling it on amethyst, it wires through to enable it on specs?

@AndreaCatania

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@azriel91

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Yeap, the thing I posted was simply to allow users to turn it on / off, instead of an always on.

@AndreaCatania AndreaCatania force-pushed the AndreaCatania:batch branch from 58d919c to 01e4cf8 Sep 12, 2019

@AndreaCatania

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

@azriel91 Yep I got what you mean, but later when I'll push the PR to fix the Transform problem it will be always on; but you are correct that active it now is confusing so I removed it completely.

@distransient distransient added this to the 0.14 milestone Sep 16, 2019

@AndreaCatania AndreaCatania force-pushed the AndreaCatania:batch branch from 01e4cf8 to 17a9109 Sep 17, 2019

@AndreaCatania AndreaCatania marked this pull request as ready for review Sep 17, 2019

@AndreaCatania AndreaCatania requested a review from azriel91 Sep 17, 2019

@codecov

This comment has been minimized.

Copy link

commented Sep 18, 2019

Codecov Report

Merging #1923 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1923   +/-   ##
=======================================
  Coverage   81.38%   81.38%           
=======================================
  Files          75       75           
  Lines        5555     5555           
=======================================
  Hits         4521     4521           
  Misses       1034     1034

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b45951...afe3d21. Read the comment docs.

@azriel91
Copy link
Member

left a comment

, oh ya, am assuming the "storage-event-control" feature is needed for #1795 ?

@AndreaCatania

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

@azriel91 yep, but I'll add it with another commit since I need to touch the Transform Systems

@AndreaCatania

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

@azriel91 I'm solving the CI issues.

@AndreaCatania

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

In local I don't have these errors, so I'm waiting your input on what to do

@azriel91

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

ah, if you want the issues locally, do a git pull -- I did a bad merge to resolve the conflict earlier (added "specs-derive" feature). Feel free to rebase and force push over that change

@AndreaCatania AndreaCatania force-pushed the AndreaCatania:batch branch 2 times, most recently from 75cb253 to b65c6b7 Sep 18, 2019

@AndreaCatania

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

I mean that after the pull it was working in local anyway so I was not sure why the CI fail.

However I've just force pushed and updated the change. Ready for review.

@AndreaCatania AndreaCatania force-pushed the AndreaCatania:batch branch from b65c6b7 to 2974091 Sep 18, 2019

@AndreaCatania

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

Alright, CI problems solved.

@AndreaCatania AndreaCatania force-pushed the AndreaCatania:batch branch from 2974091 to 9c4089c Sep 18, 2019

@AndreaCatania AndreaCatania reopened this Sep 18, 2019

@AndreaCatania

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

The CI failed due to the coverage.

@AndreaCatania AndreaCatania requested a review from azriel91 Sep 18, 2019

@azriel91
Copy link
Member

left a comment

Yay

@azriel91

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

Coverage isn't consistent, so it's more of a reminder than a hard check

@azriel91

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

bors try

(oh ya, that's a segmentation fault, I couldn't reproduce it so it's hard to figure out what's wrong)

bors bot added a commit that referenced this pull request Sep 18, 2019
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

@azriel91

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

I've fixed the book failures, grab this commit:

git remote add azriel91 git@github.com:azriel91/amethyst.git
git fetch azriel91 batch
git cherry-pick azriel91/batch

(I couldn't push to your remote directly)

@AndreaCatania

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@azriel91 done

@AndreaCatania AndreaCatania force-pushed the AndreaCatania:batch branch from c316e93 to 53d51e3 Sep 19, 2019

@AndreaCatania AndreaCatania requested a review from distransient Sep 19, 2019

@AndreaCatania AndreaCatania force-pushed the AndreaCatania:batch branch from 53d51e3 to d3f50e4 Sep 19, 2019

@@ -24,8 +24,7 @@ If you intend to include a [`Component`] that has not yet got a corresponding [`
Error,
};
use serde::{Deserialize, Serialize};
use specs_derive::Component;
```
```

This comment has been minimized.

Copy link
@azriel91

azriel91 Sep 19, 2019

Member

ah sorry, I probably introduced this

This comment has been minimized.

Copy link
@distransient

distransient Sep 20, 2019

Member

I'm going to add my approval here because family is in town but pls fix this and push and azriel can merge it for you :)

This comment has been minimized.

Copy link
@AndreaCatania

AndreaCatania Sep 20, 2019

Author Contributor

Done

@distransient

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

bors try

bors bot added a commit that referenced this pull request Sep 20, 2019
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

@azriel91

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

bors r=azriel91,distransient

bors bot added a commit that referenced this pull request Sep 20, 2019
Merge #1923
1923: Updated SPECS r=azriel91,distransient a=AndreaCatania

## Description

Updated SPECS to the last version and using the new flagged storage feature

## PR Checklist
- [x] Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme.

If this modified or created any rs files:

- [x] Ran `cargo +stable fmt --all`
- [x] Ran `cargo clippy --all --features "empty"`
- [x] Ran `cargo test --all --features "empty"`


Co-authored-by: Andrea Catania <info@andreacatania.com>
Co-authored-by: Azriel Hoh <azriel91@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

@bors bors bot merged commit afe3d21 into amethyst:master Sep 20, 2019

2 checks passed

bors Build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@AndreaCatania AndreaCatania deleted the AndreaCatania:batch branch Sep 20, 2019

@AndreaCatania

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

ty

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.