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

Emissive is now LinearRgba on StandardMaterial #13352

Merged

Conversation

andristarr
Copy link
Contributor

StandardMaterial stores a LinearRgba instead of a Color for emissive

Fixes #13212

Copy link
Member

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

Choose a reason for hiding this comment

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

There we go, that looks correct now. What needs to be done still?

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A simple quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy C-Code-Quality A section of code that is hard to understand or change X-Uncontroversial This work is generally agreed upon and removed C-Usability A simple quality-of-life change that makes Bevy easier to use labels May 13, 2024
@andristarr andristarr force-pushed the standardmaterial.linearrgba branch 2 times, most recently from 7fde0d9 to 3bbf3bd Compare May 13, 2024 18:26
@andristarr andristarr force-pushed the standardmaterial.linearrgba branch from 3bbf3bd to 04dd81a Compare May 13, 2024 19:26
@andristarr
Copy link
Contributor Author

Just need to change the wording around the emissive so its not confusing

@andristarr andristarr force-pushed the standardmaterial.linearrgba branch 2 times, most recently from ca4fd0b to 6ef0701 Compare May 14, 2024 07:59
@andristarr andristarr marked this pull request as ready for review May 14, 2024 14:50
@andristarr
Copy link
Contributor Author

This should be ready for review @alice-i-cecile

@alice-i-cecile alice-i-cecile added this to the 0.14 milestone May 14, 2024
@alice-i-cecile alice-i-cecile added the A-Color Color spaces and color math label May 14, 2024
examples/3d/3d_scene.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Looks good! just a few comments

@alice-i-cecile alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label May 16, 2024
@andristarr
Copy link
Contributor Author

Sorry, i was on vacation and will get to this soon!

@alice-i-cecile
Copy link
Member

Thanks for letting us know :) I hope you had a lovely vacation!

@andristarr andristarr force-pushed the standardmaterial.linearrgba branch from ea3526f to c77b5d6 Compare May 24, 2024 10:18
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 24, 2024
@andristarr andristarr force-pushed the standardmaterial.linearrgba branch 3 times, most recently from 8074e2e to fc21890 Compare May 24, 2024 13:42
@andristarr
Copy link
Contributor Author

I think your comments have been addressed, please re-review when you have time @alice-i-cecile

Copy link
Member

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

Choose a reason for hiding this comment

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

The values in the bloom_3d example got messed up. Other than that, this looks great :)

@andristarr andristarr force-pushed the standardmaterial.linearrgba branch from fc21890 to 414833f Compare May 24, 2024 15:22
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 24, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 24, 2024
Merged via the queue into bevyengine:main with commit 44c0325 May 24, 2024
28 checks passed
@mockersf
Copy link
Member

This PR broke bloom, as you can see if you run example bloom_3d.
#13502 fixes that

github-merge-queue bot pushed a commit that referenced this pull request May 25, 2024
)

# Objective

- #13352 broke bloom in 3d

## Solution

- Use the correct value for `emissive` in `StandardMaterial`. It's
computed just above but unused

https://github.com/bevyengine/bevy/blob/d87505899fc6adcb1a26550e921b0f5b0c351c57/crates/bevy_pbr/src/pbr_material.rs#L975-L976

## Testing

- Run example `bloom_3d`
@IceSentry
Copy link
Contributor

Why did this PR change a bunch of examples to not use the From<Color> impl for StandardMaterial?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Color Color spaces and color math A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StandardMaterial::emissive field should use LinearRgba
6 participants