/amethyst

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 BlendTargets to PSO #411

Merged
merged 3 commits into from Oct 14, 2017

Conversation

Reviewers

@Rhuagh Rhuagh

@torkleyy torkleyy

@omni-viral omni-viral

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
4 participants
@Xaeroxe @omni-viral @Rhuagh @torkleyy
@Xaeroxe
Member

Xaeroxe commented Oct 13, 2017

This PR has the intention of adding blending targets to the pipeline. While this is a good first step towards implementing transparency in our renderer more work is required. This PR is being opened early for initial feedback.

@Xaeroxe Xaeroxe added project: rendering status: working type: feature labels Oct 13, 2017

@omni-viral

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Oct 13, 2017

Member

Add blender targets to the Effect and it's ready to merge. 👍
Member

omni-viral commented Oct 13, 2017

Add blender targets to the Effect and it's ready to merge. 👍
amethyst_renderer/src/pipe/effect/pso.rs
@@ -79,6 +82,18 @@ impl<'d> PipelineInit for Init<'d> {
meta.out_colors.push(meta_color);
}
+ for blend in self.out_blends.iter() {
+ let mut meta_blend = <BlendTarget as DataLink<'d>>::new();
+ for info in info.outputs.iter() {

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 13, 2017

Member

Nit: no need for iter, use reference instead.
@torkleyy

torkleyy Oct 13, 2017

Member

Nit: no need for iter, use reference instead.
amethyst_renderer/src/pipe/effect/pso.rs
+ if let Some(res) = meta_blend.link_output(info, blend) {
+ let d = res.map_err(|e| InitError::PixelExport(info.name.as_str(), Some(e)))?;
+ desc.color_targets[info.slot as usize] = Some(d);
+ break;

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 13, 2017

Member

You could just use find instead of a for loop.
@torkleyy

torkleyy Oct 13, 2017

Member

You could just use find instead of a for loop.

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 13, 2017

Member

This ends up being less efficient, as you then have to call link_output twice.
@Xaeroxe

Xaeroxe Oct 13, 2017

Member

This ends up being less efficient, as you then have to call link_output twice.
amethyst_renderer/src/pipe/effect/pso.rs
@@ -96,6 +111,16 @@ impl<'d> PipelineInit for Init<'d> {
}
meta.out_colors.push(meta_color);
}
+
+ for blend in self.out_blends.iter() {

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 13, 2017

Member

Same as above
@torkleyy

torkleyy Oct 13, 2017

Member

Same as above

@Xaeroxe Xaeroxe requested a review from omni-viral Oct 13, 2017

@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 13, 2017

Member

r? @omni-viral

I added the blend targets to the effects and addressed @torkleyy 's nits.
Member

Xaeroxe commented Oct 13, 2017

r? @omni-viral

I added the blend targets to the effects and addressed @torkleyy 's nits.
@Rhuagh

Rhuagh approved these changes Oct 13, 2017
View changes

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 13, 2017

Member

Is this still WIP?
Member

Rhuagh commented Oct 13, 2017

Is this still WIP?
@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 13, 2017

Member

So, I have limited understanding of gfx so I'm not entirely certain if what I've provided is adequate to make passes with blending, but I think it is. The end goal of the PR is to make a UI pass possible.
Member

Xaeroxe commented Oct 13, 2017

So, I have limited understanding of gfx so I'm not entirely certain if what I've provided is adequate to make passes with blending, but I think it is. The end goal of the PR is to make a UI pass possible.

@Xaeroxe Xaeroxe removed the status: working label Oct 13, 2017

@Xaeroxe Xaeroxe changed the title from [WIP] Add BlendTargets to PSO to Add BlendTargets to PSO Oct 13, 2017

amethyst_renderer/src/pipe/effect/mod.rs
+ .expect("Failed to update buffer (TODO: replace expect)");
+ }
+ None => {
+ eprintln!("WARNING: Buffer update for effect failed! Buffer not found: {:?}",

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 13, 2017

Member

two whitespaces ("failed! Buffer")?
@torkleyy

torkleyy Oct 13, 2017

Member

two whitespaces ("failed! Buffer")?
amethyst_renderer/src/pipe/effect/mod.rs
+ enc.update_constant_buffer::<T>(unsafe { mem::transmute(raw) }, &data)
+ }
+ None => {
+ eprintln!("WARNING: Buffer update for effect failed! Buffer not found: {:?}",

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 13, 2017

Member

Same as above
@torkleyy

torkleyy Oct 13, 2017

Member

Same as above
@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 13, 2017

Member

Btw, the mut has already been removed by my PR, try rebasing onto develop.
Member

torkleyy commented Oct 13, 2017

Btw, the mut has already been removed by my PR, try rebasing onto develop.
@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 13, 2017

Member

Hmm, seems something went wrong :(
Member

torkleyy commented Oct 13, 2017

Hmm, seems something went wrong :(
@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 13, 2017

Member

@torkleyy Sometimes I rebase onto the wrong thing and stuff looks funny. I think I fixed it now.
Member

Xaeroxe commented Oct 13, 2017

@torkleyy Sometimes I rebase onto the wrong thing and stuff looks funny. I think I fixed it now.
amethyst_renderer/src/pipe/effect/mod.rs
@@ -275,6 +294,8 @@ impl<'a> EffectBuilder<'a> {
data.out_colors
.extend(self.out.color_bufs().iter().map(|cb| cb.as_output.clone()));
+ data.out_blends
+ .extend(self.out.color_bufs().iter().map(|cb| cb.as_output.clone()));

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 13, 2017

Member

Can't we use .cloned() here?
@torkleyy

torkleyy Oct 13, 2017

Member

Can't we use .cloned() here?
@Xaeroxe
Use cloned where possible
Loading status checks…
f937104
@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Oct 14, 2017

Member

bors r+
Member

Xaeroxe commented Oct 14, 2017

bors r+

bors bot added a commit that referenced this pull request Oct 14, 2017

@bors[bot]
Merge #411 
411: Add BlendTargets to PSO r=Xaeroxe a=Xaeroxe

This PR has the intention of adding blending targets to the pipeline.  While this is a good first step towards implementing transparency in our renderer more work is required.  This PR is being opened early for initial feedback.
Loading status checks…
8fe7175
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot Oct 14, 2017

Contributor

Build succeeded
Contributor

bors bot commented Oct 14, 2017

Build succeeded

@bors bors bot merged commit f937104 into amethyst:develop Oct 14, 2017
3 checks passed

3 checks passed

bors Build succeeded
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Xaeroxe Xaeroxe deleted the Xaeroxe:blend branch Oct 18, 2017

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