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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - change how to select bevy-glsl-to-spirv or shaderc #1819

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - change how to select bevy-glsl-to-spirv or shaderc #1819

wants to merge 4 commits into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Apr 5, 2021 •
edited
Loading

cfg for bevy-glsl-to-spirv use now mimics https://github.com/cart/glsl-to-spirv/blob/master/Cargo.toml

fixes #898
fixes #1348
fixes #1942
fixes #1078

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change labels Apr 5, 2021
MinerSebas
MinerSebas reviewed Apr 11, 2021
View reviewed changes
crates/bevy_render/src/shader/shader.rs Outdated
Comment on lines 37 to 39
all(target_arch = "x86", target_os = "windows", target_env = "msvc"),
all(target_arch = "x86_64", target_os = "windows", target_env = "gnu"),
all(target_arch = "x86", target_os = "windows", target_env = "gnu")
Copy link
Contributor

Choose a reason for hiding this comment

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

The Version of glsl-to-spirv on crates.io, does not yet have the Support for these three Targets.
Either they should be removed for now, or Cart should release a new version of glsl-to-spirv.

Copy link
Member

Choose a reason for hiding this comment

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

I just tried publishing the new version, but the new crate structure causes problems: cart/glsl-to-spirv#12.

Short term if we want to merge this asap we should assume we need to use the current version here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed them

Copy link
Member Author

@mockersf mockersf Apr 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

if we want to merge this

I opened this after seeing several people with issue around that, but none in the last few days.

@MinerSebas
Copy link
Contributor

MinerSebas commented Apr 16, 2021 •
edited
Loading

Could you edit the PR Description to include Fixes #1348 and Fixes #898 so that #1348 and #898 are automatically closed when this is merged.

Edit: Please do the same with the new #1942

MinerSebas
MinerSebas reviewed Apr 16, 2021
View reviewed changes
crates/bevy_render/src/shader/shader.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/shader/shader.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/shader/shader.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/shader/shader.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/shader/shader.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/shader/shader.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/shader/shader.rs Outdated Show resolved Hide resolved
crates/bevy_render/Cargo.toml Outdated Show resolved Hide resolved
@MinerSebas MinerSebas mentioned this pull request Apr 16, 2021
Closed
mockersf and others added 3 commits April 16, 2021 22:19
@mockersf
change how to select bevy-glsl-to-spirv or shaderc
6cf0039
@mockersf
remove unsupported targets
376e2e1
@mockersf @MinerSebas
androidabi on armv7
a46c35a 
Co-Authored-By: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
@skhameneh
Copy link

Confirming a46c35a fixes #1942.

MinerSebas
MinerSebas reviewed Apr 16, 2021
View reviewed changes
Copy link
Contributor

@MinerSebas MinerSebas left a comment

Choose a reason for hiding this comment

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

Missed to suggest these two. 😅

crates/bevy_render/Cargo.toml Outdated Show resolved Hide resolved
crates/bevy_render/Cargo.toml Outdated Show resolved Hide resolved
@mockersf @MinerSebas
remove unsupported windows from toml
e19b8cf 
Co-Authored-By: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
@MinerSebas
Copy link
Contributor

@mockersf I found another Issue that can be closed when this merges: #1078

@cart
Copy link
Member

cart commented Apr 19, 2021

This looks good to me. Not a huge fan of the conditional compilation complexity + large duplicated target blocks that need to be kept in sync, but this does seem like the pragmatic choice for now.

@cart
Copy link
Member

cart commented Apr 19, 2021

bors r+

bors bot pushed a commit that referenced this pull request Apr 19, 2021
@mockersf
change how to select bevy-glsl-to-spirv or shaderc (#1819)
f1ddd7a 
`cfg` for `bevy-glsl-to-spirv` use now mimics https://github.com/cart/glsl-to-spirv/blob/master/Cargo.toml

fixes #898 
fixes #1348 
fixes #1942 
fixes #1078
@bors
Copy link
Contributor

bors bot commented Apr 19, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title change how to select bevy-glsl-to-spirv or shaderc [Merged by Bors] - change how to select bevy-glsl-to-spirv or shaderc Apr 19, 2021
@bors bors bot closed this Apr 19, 2021
This was referenced Jun 8, 2021
Closed
Closed
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
@mockersf @ostwilkens
change how to select bevy-glsl-to-spirv or shaderc (bevyengine#1819)
0f04d08 
`cfg` for `bevy-glsl-to-spirv` use now mimics https://github.com/cart/glsl-to-spirv/blob/master/Cargo.toml

fixes bevyengine#898 
fixes bevyengine#1348 
fixes bevyengine#1942 
fixes bevyengine#1078
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cart cart cart left review comments

@MinerSebas MinerSebas MinerSebas left review comments

Assignees
No one assigned
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
Milestone
No milestone
5 participants
@mockersf @MinerSebas @skhameneh @cart @alice-i-cecile