Skip to content

[Merged by Bors] - Added TryFrom for VertexAttributeValues - #1963

Closed
simensgreen wants to merge 10 commits into
bevyengine:mainfrom
simensgreen:vertex_attrib_TryInto
Closed

simensgreen wants to merge 10 commits into
bevyengine:mainfrom
simensgreen:vertex_attrib_TryInto

Conversation

@simensgreen

Copy link
Copy Markdown
Contributor

This implementations allows you
convert std::vec::Vec to VertexAttributeValues::T and back.

Examples

use std::convert::TryInto;
use bevy_render::mesh::VertexAttributeValues;

// creating vector of values
let before = vec![[0_u32; 4]; 10];
let values = VertexAttributeValues::from(before.clone());
let after: Vec<[u32; 4]> = values.try_into().unwrap();

assert_eq!(before, after);

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Rendering Drawing game state to the screen labels Apr 19, 2021

@alice-i-cecile alice-i-cecile left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like splitting this out into a conversions.rs file! And seems like helpful functionality with great tests.

@simensgreen

Copy link
Copy Markdown
Contributor Author

I like splitting this out into a conversions.rs file! And seems like helpful functionality with great tests.

Thanks

Comment thread crates/bevy_render/src/mesh/mesh/conversions.rs Outdated
@inodentry

Copy link
Copy Markdown
Contributor

You should impl TryFrom instead of TryInto. The Rust std library provides TryInto impls for anything that impls TryFrom, but not the other way around.

@simensgreen

Copy link
Copy Markdown
Contributor Author

You should impl TryFrom instead of TryInto. The Rust std library provides TryInto impls for anything that impls TryFrom, but not the other way around.

Yes, I completely agree, I will do it now, thank you

- `Short2`
- `Short2Norm`
- `Ushort2`
- `Ushort2Norm`
- `Short4`
- `Short4Norm`
- `Ushort4`
- `Ushort4Norm`
- `Char2`
- `Char2Norm`
- `Uchar2`
- `Uchar2Norm`
- `Char4`
- `Char4Norm`
- `Uchar4`
@alice-i-cecile

Copy link
Copy Markdown
Member

Can you change the PR name? It'll get squashed down into a single commit with that name when we merge and it's now backwards :)

@simensgreen simensgreen changed the title Added TryInto for VertexAttributeValues Added TryFrom for VertexAttributeValues Apr 20, 2021
VertexAttributeValues::Char4(..) => "VertexAttributeValues::Char4",
VertexAttributeValues::Char4Norm(..) => "VertexAttributeValues::Char4Norm",
VertexAttributeValues::Uchar4(..) => "VertexAttributeValues::Uchar4",
VertexAttributeValues::Uchar4Norm(..) => "VertexAttributeValues::Uchar4Norm",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not added a _ placeholder so that whoever adds new formats will take care of conversions

Comment thread crates/bevy_render/src/mesh/mesh/conversions.rs Outdated
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Comment thread crates/bevy_render/src/mesh/mesh/conversions.rs Outdated
cart and others added 2 commits April 20, 2021 13:29
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
@cart

cart commented Apr 20, 2021

Copy link
Copy Markdown
Member

Looks good to me! I moved the tests into their own submodule to ensure imports aren't unused when building without tests.

I'd also like to remove the manual "type name strings" everywhere. But I understand that rust doesn't give us the tools we need to get enum variant names.

We could use a crate like strum to do this, but thats a bit too big for such a small need. I think I'll just whip up a quick derive macro (which we can then use and/or copy for bevy_reflect enum support).

For now, lets just merge this!

@cart

cart commented Apr 20, 2021

Copy link
Copy Markdown
Member

bors r+

bors Bot pushed a commit that referenced this pull request Apr 20, 2021
This implementations allows you
convert std::vec::Vec<T> to VertexAttributeValues::T and back.

# Examples

```rust
use std::convert::TryInto;
use bevy_render::mesh::VertexAttributeValues;

// creating vector of values
let before = vec![[0_u32; 4]; 10];
let values = VertexAttributeValues::from(before.clone());
let after: Vec<[u32; 4]> = values.try_into().unwrap();

assert_eq!(before, after);
```

Co-authored-by: aloucks <aloucks@cofront.net>
Co-authored-by: simens_green <34134129+simensgreen@users.noreply.github.com>
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors

bors Bot commented Apr 20, 2021

Copy link
Copy Markdown

Build failed:

@cart

cart commented Apr 20, 2021

Copy link
Copy Markdown
Member

bors r+

bors Bot pushed a commit that referenced this pull request Apr 20, 2021
This implementations allows you
convert std::vec::Vec<T> to VertexAttributeValues::T and back.

# Examples

```rust
use std::convert::TryInto;
use bevy_render::mesh::VertexAttributeValues;

// creating vector of values
let before = vec![[0_u32; 4]; 10];
let values = VertexAttributeValues::from(before.clone());
let after: Vec<[u32; 4]> = values.try_into().unwrap();

assert_eq!(before, after);
```

Co-authored-by: aloucks <aloucks@cofront.net>
Co-authored-by: simens_green <34134129+simensgreen@users.noreply.github.com>
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors

bors Bot commented Apr 20, 2021

Copy link
Copy Markdown

@bors bors Bot changed the title Added TryFrom for VertexAttributeValues [Merged by Bors] - Added TryFrom for VertexAttributeValues Apr 20, 2021
@bors bors Bot closed this Apr 20, 2021
bors Bot pushed a commit that referenced this pull request Apr 21, 2021
There are cases where we want an enum variant name. Right now the only way to do that with rust's std is to derive Debug, but this will also print out the variant's fields. This creates the unfortunate situation where we need to manually write out each variant's string name (ex: in #1963), which is both boilerplate-ey and error-prone. Crates such as `strum` exist for this reason, but it includes a lot of code and complexity that we don't need.

This adds a dead-simple `EnumVariantMeta` derive that exposes `enum_variant_index` and `enum_variant_name` functions. This allows us to make cases like #1963 much cleaner (see the second commit). We might also be able to reuse this logic for `bevy_reflect` enum derives.
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
This implementations allows you
convert std::vec::Vec<T> to VertexAttributeValues::T and back.

# Examples

```rust
use std::convert::TryInto;
use bevy_render::mesh::VertexAttributeValues;

// creating vector of values
let before = vec![[0_u32; 4]; 10];
let values = VertexAttributeValues::from(before.clone());
let after: Vec<[u32; 4]> = values.try_into().unwrap();

assert_eq!(before, after);
```

Co-authored-by: aloucks <aloucks@cofront.net>
Co-authored-by: simens_green <34134129+simensgreen@users.noreply.github.com>
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
There are cases where we want an enum variant name. Right now the only way to do that with rust's std is to derive Debug, but this will also print out the variant's fields. This creates the unfortunate situation where we need to manually write out each variant's string name (ex: in bevyengine#1963), which is both boilerplate-ey and error-prone. Crates such as `strum` exist for this reason, but it includes a lot of code and complexity that we don't need.

This adds a dead-simple `EnumVariantMeta` derive that exposes `enum_variant_index` and `enum_variant_name` functions. This allows us to make cases like bevyengine#1963 much cleaner (see the second commit). We might also be able to reuse this logic for `bevy_reflect` enum derives.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants