Skip to content

/gfx

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 optional serialization support, version bumps, formatting fixes #1234

Merged
merged 1 commit into from Apr 21, 2017

Conversation

@ebkalderon
Copy link
Contributor

commented Apr 21, 2017 •
edited

Note: This is the season finale for our two-part series. Prepare for gfx 0.15! 🎉

Added

  • Add serialize feature to gfx_app and gfx_core, courtesy of draw_state 0.7.0 (fixes #1229)
  • Added API docs to Copy{Buffer,BufferTexture,TextureBuffer}Result types

Changed

  • Minor reformatting and alphabetical ordering of #[derive] statements
  • Switch one manual Eq implementation to #[derive] instead
  • Encoder now uses #[derive(Debug)]
  • Bumped minor version numbers of affected crates (gfx is now up to 0.15.0)
  • Updated several third-party dependencies
  • Build against Rust 1.16.0 on AppVeyor

Fixed

  • Expose CopyError and Copy{Buffer,BufferTexture,TextureBuffer}Result types from gfx

CC @kvark
@torkleyy

torkleyy reviewed Apr 21, 2017
View changes

Copy link
Contributor

left a comment

Seems we're getting serialization support for several libraries at the moment, nice. I just took a quick look as I don't have much time and I'm not very familiar with this code base yet.
.travis.yml Outdated
@@ -51,7 +51,7 @@ script:
- if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then export LD_LIBRARY_PATH=$LIBRARY_PATH ; fi
- if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then cargo build --features vulkan; else cargo build; fi
- if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then cargo build --features metal; else cargo build; fi
- cargo test --all
- cargo test --all --features serialize

This comment has been minimized.

Sign in to view
Copy link
@torkleyy

torkleyy Apr 21, 2017

Contributor

Maybe test with and without the feature?

This comment has been minimized.

Sign in to view
Copy link
@ebkalderon

ebkalderon Apr 21, 2017

Author Contributor

Given that the feature only makes the types derive from Serialize /Deserialize and nothing more, it just has to compile correctly to pass. But if you really want, I can duplicate the line to re-run all tests without it.

This comment has been minimized.

Sign in to view
Copy link
@torkleyy

torkleyy Apr 21, 2017

Contributor

Just a thought, you can leave it as is. We could also add the extra test later, should we implement anything more advanced.

This comment has been minimized.

Sign in to view
Copy link
@ebkalderon

ebkalderon Apr 21, 2017

Author Contributor

Good point! I think I'll add it in.
appveyor.yml Outdated
test_script:
- cargo test --all --features vulkan
- cargo test --all --features "vulkan serialize"

This comment has been minimized.

Sign in to view
Copy link
@torkleyy

torkleyy Apr 21, 2017

Contributor

Same as in .travis.yml
@ebkalderon

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Apr 21, 2017

@kvark Alright, I've updated the PR and it's ready for review!
@kvark

kvark requested changes Apr 21, 2017
View changes

Copy link
Member

left a comment

Thanks, looks great! A few fixes are needed before we proceed.
src/core/src/handle.rs Outdated
pub struct Buffer<R: Resources, T>(RawBuffer<R>, PhantomData<T>);

impl<R: Resources, T> cmp::PartialEq for Buffer<R, T> {
fn eq(&self, other: &Self) -> bool { self.0.eq(&other.0) }
}

impl<R: Resources, T> cmp::Eq for Buffer<R, T> {}

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Apr 21, 2017

Member

we shouldn't be removing this line. The difference with derive is that we don't require T: Eq here, which is essential

This comment has been minimized.

Sign in to view
Copy link
@ebkalderon

ebkalderon Apr 21, 2017

Author Contributor

Oh, I see. I'll revert this change, then.
Cargo.toml Outdated
winit = "0.5.1"
gfx_core = { path = "src/core", version = "0.6" }
env_logger = "0.4"
glutin = "0.7"

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Apr 21, 2017

Member

you shouldn't strip the patch version from here - it is likely for a reason

This comment has been minimized.

Sign in to view
Copy link
@ebkalderon

ebkalderon Apr 21, 2017

Author Contributor

According to semantic versioning, the patch number is for backwards-compatible bug fixes only. By removing the patch version from the Cargo.toml, we can get these bug fixes whenever they come out, so we don't have to manually increment the version number over and over again.

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Apr 21, 2017 •
edited

Member

I think you are misreading the spec slightly. For 0.x.y, AFAIK, x is considered a "major" and y as "minor". Adding features just requires you to bump the patch version then (a "minor"), as long as it's backwards compatible. If something depends on new features, it needs to have the patch version specified for that minimal version (what we have). cargo update would still use the newer patch versions, if available, but it will guarantee we'll not get lower ones.
src/render/Cargo.toml Outdated
@@ -30,9 +30,10 @@ name = "gfx"
path = "src/lib.rs"

[features]
serialize = ["draw_state/serialize"]

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Apr 21, 2017

Member

shouldn't it also say gfx_core/serialize here?

This comment has been minimized.

Sign in to view
Copy link
@ebkalderon

ebkalderon Apr 21, 2017

Author Contributor

Correct, I'll fix that.
src/window/dxgi/Cargo.toml Outdated
winit = "0.5.2"
gfx_core = { path = "../../core", version = "0.6" }
gfx_device_dx11 = { path = "../../backend/dx11", version = "0.5" }
winit = "0.5"

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Apr 21, 2017

Member

again, please don't strip this patch requirement
Cargo.toml Outdated
@@ -13,27 +13,27 @@ default = []
sdl = ["gfx_window_sdl"]
vulkan = ["gfx_device_vulkan", "gfx_device_vulkanll", "gfx_window_vulkan"]
metal = ["gfx_device_metal", "gfx_window_metal"]
serialize = ["gfx_core/serialize"]

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Apr 21, 2017

Member

should probably also say gfx/serialize

This comment has been minimized.

Sign in to view
Copy link
@ebkalderon

ebkalderon Apr 21, 2017

Author Contributor

Correct, I'll fix that.
@ebkalderon
Add optional serialization support, version bumps, formatting fixes 
Update CHANGELOG.md

Update appveyor.yml

Build against Rust 1.16.0 on AppVeyor

Update .travis.yml

Update appveyor.yml

Revert derive(Eq) macro for Buffer

Correct draw_state/serialize to gfx_core/serialize

Correct gfx_core/serialize to gfx/serialize

Restore patch numbers for Cargo.toml

Restore patch requirement in Cargo.toml

Update appveyor.yml, .travis.yml, Cargo.toml
Loading status checks…
35e5ad0
@kvark

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 21, 2017

@homu

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 21, 2017

📌 Commit 35e5ad0 has been approved by kvark
@homu

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 21, 2017

⌛️ Testing commit 35e5ad0 with merge 6134302...

homu added a commit that referenced this pull request Apr 21, 2017

@homu
Auto merge of #1234 - ebkalderon:serde, r=kvark 
Add optional serialization support, version bumps, formatting fixes

> Note: This is the season finale for our two-part series. Prepare for `gfx` 0.15! 🎉

### Added
* Add `serialize` feature to `gfx_app` and `gfx_core`, courtesy of `draw_state` 0.7.0 (fixes #1229)
* Added API docs to `Copy{Buffer,BufferTexture,TextureBuffer}Result` types

### Changed
* Minor reformatting and alphabetical ordering of `#[derive]` statements
* Switch one manual `Eq` implementation to `#[derive]` instead
* Encoder now uses `#[derive(Debug)]`
* Bumped minor version numbers of affected crates (`gfx` is now up to 0.15.0)
* Updated several third-party dependencies
* Build against Rust 1.16.0 on AppVeyor

### Fixed
* Expose `CopyError` and `Copy{Buffer,BufferTexture,TextureBuffer}Result` types from `gfx`

CC @kvark
Loading status checks…
6134302
@ebkalderon

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Apr 21, 2017

@kvark So the PR is complete! Commenting here, as requested.
@homu

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Apr 21, 2017

☀️ Test successful - status

@homu homu merged commit 35e5ad0 into gfx-rs:master Apr 21, 2017
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@homu homu referenced this pull request Apr 21, 2017

Merged

Adding more caching to GL backend. (#1221) #1233

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

@kvark kvark

@torkleyy torkleyy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@ebkalderon @kvark @homu @torkleyy
You can’t perform that action at this time.