Skip to content

[Merged by Bors] - bevy_reflect: Added PartialEq to reflected f32 & f64 - #4217

Closed
MrGVSV wants to merge 5 commits into
bevyengine:mainfrom
MrGVSV:reflect-floating-partial-eq
Closed

MrGVSV wants to merge 5 commits into
bevyengine:mainfrom
MrGVSV:reflect-floating-partial-eq

Conversation

@MrGVSV

@MrGVSV MrGVSV commented Mar 15, 2022 •

Copy link
Copy Markdown
Member

Objective

Comparing two reflected floating points would always fail:

let a: &dyn Reflect = &1.23_f32;
let b: &dyn Reflect = &1.23_f32;

// Panics:
assert!(a.reflect_partial_eq(b).unwrap_or_default());

The comparison returns None since f32 (and f64) does not have a reflected PartialEq implementation.

Solution

Include PartialEq in the impl_reflect_value! macro call for both f32 and f64.

Hash is still excluded since neither implement Hash.

Also added equality tests for some of the common types from std (including f32).

@github-actions github-actions Bot added the S-Needs-Triage This issue needs to be labelled label Mar 15, 2022
@james7132 james7132 added C-Feature A new feature, making something new possible A-Reflection Runtime information about types and removed S-Needs-Triage This issue needs to be labelled labels Mar 15, 2022
Comment thread crates/bevy_reflect/src/impls/std.rs Outdated
@MrGVSV MrGVSV changed the title Added PartialEq to reflected f32 & f64 bevy_reflect: Added PartialEq to reflected f32 & f64 Mar 28, 2022
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 22, 2022
@alice-i-cecile

Copy link
Copy Markdown
Member

bors r+

bors Bot pushed a commit that referenced this pull request Apr 26, 2022
# Objective

Comparing two reflected floating points would always fail:

```rust
let a: &dyn Reflect = &1.23_f32;
let b: &dyn Reflect = &1.23_f32;

// Panics:
assert!(a.reflect_partial_eq(b).unwrap_or_default());
```

The comparison returns `None` since `f32` (and `f64`) does not have a reflected `PartialEq` implementation.

## Solution

Include `PartialEq` in the `impl_reflect_value!` macro call for both `f32` and `f64`.

`Hash` is still excluded since neither implement `Hash`.

Also added equality tests for some of the common types from `std` (including `f32`).
@bors bors Bot changed the title bevy_reflect: Added PartialEq to reflected f32 & f64 [Merged by Bors] - bevy_reflect: Added PartialEq to reflected f32 & f64 Apr 26, 2022
@bors bors Bot closed this Apr 26, 2022
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
…e#4217)

# Objective

Comparing two reflected floating points would always fail:

```rust
let a: &dyn Reflect = &1.23_f32;
let b: &dyn Reflect = &1.23_f32;

// Panics:
assert!(a.reflect_partial_eq(b).unwrap_or_default());
```

The comparison returns `None` since `f32` (and `f64`) does not have a reflected `PartialEq` implementation.

## Solution

Include `PartialEq` in the `impl_reflect_value!` macro call for both `f32` and `f64`.

`Hash` is still excluded since neither implement `Hash`.

Also added equality tests for some of the common types from `std` (including `f32`).
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…e#4217)

# Objective

Comparing two reflected floating points would always fail:

```rust
let a: &dyn Reflect = &1.23_f32;
let b: &dyn Reflect = &1.23_f32;

// Panics:
assert!(a.reflect_partial_eq(b).unwrap_or_default());
```

The comparison returns `None` since `f32` (and `f64`) does not have a reflected `PartialEq` implementation.

## Solution

Include `PartialEq` in the `impl_reflect_value!` macro call for both `f32` and `f64`.

`Hash` is still excluded since neither implement `Hash`.

Also added equality tests for some of the common types from `std` (including `f32`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Reflection Runtime information about types C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants