/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

feat: Support transparency in core passes #574

Merged
merged 1 commit into from Feb 18, 2018

Conversation

Reviewers

@jojolepro jojolepro

@Xaeroxe Xaeroxe

@torkleyy torkleyy

@omni-viral omni-viral

@Aceeri Aceeri

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

Rhuagh commented Feb 14, 2018 •
edited by torkleyy
Edited 1 time
  • @torkleyy torkleyy edited Feb 14, 2018 (most recent)

This change is Reviewable

@Rhuagh Rhuagh added type: improvement project: rendering status: working labels Feb 14, 2018

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 14, 2018

Member

Only one pass so far, will continue tomorrow.
Member

Rhuagh commented Feb 14, 2018

Only one pass so far, will continue tomorrow.
@Xaeroxe

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Feb 14, 2018

Member

I like this so far, of course we'll still need to sort the entities back to front. I'm still not sure how to do this without recalculating the distance between every transparent mesh and the camera every frame though. We might just have to accept the performance hit.
Member

Xaeroxe commented Feb 14, 2018

I like this so far, of course we'll still need to sort the entities back to front. I'm still not sure how to do this without recalculating the distance between every transparent mesh and the camera every frame though. We might just have to accept the performance hit.

@Xaeroxe Xaeroxe requested review from omni-viral, Aceeri and torkleyy Feb 14, 2018

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 15, 2018

Member

PBM and bundle updates left.
Member

Rhuagh commented Feb 15, 2018

PBM and bundle updates left.
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 15, 2018

Member

The current sorting system does what @Xaeroxe said, it recomputes every frame. This should be fairly easy to fix when we get Tracked in specs released.
Member

Rhuagh commented Feb 15, 2018

The current sorting system does what @Xaeroxe said, it recomputes every frame. This should be fairly easy to fix when we get Tracked in specs released.

@Rhuagh Rhuagh changed the title from [WIP] feat: Support transparency in core passes to feat: Support transparency in core passes Feb 15, 2018

@Rhuagh Rhuagh added status: ready and removed status: working labels Feb 15, 2018

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Feb 16, 2018

Member

Reviewed 3 of 13 files at r1.
Review status: 3 of 13 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable
Member

torkleyy commented Feb 16, 2018

Reviewed 3 of 13 files at r1.
Review status: 3 of 13 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable
amethyst_renderer/src/transparent.rs
+ )
+ })
+ .filter(|c| c.3.dot(camera_forward) > 0.) // filter entities behind the camera
+ .collect();

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Feb 16, 2018

Member

Should use a clear() and extend() here to re-use the allocation.
@Xaeroxe

Xaeroxe Feb 16, 2018

Member

Should use a clear() and extend() here to re-use the allocation.
@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Feb 16, 2018

Member

Reviewed 2 of 13 files at r1.
Review status: 5 of 13 files reviewed at latest revision, 2 unresolved discussions.

amethyst_renderer/src/pass/util.rs, line 149 at r1 (raw file):

        None => return,
    };
    if let None = material {

Should be is_none; can be || together with the if-block below.

Comments from Reviewable
Member

torkleyy commented Feb 16, 2018

Reviewed 2 of 13 files at r1.
Review status: 5 of 13 files reviewed at latest revision, 2 unresolved discussions.

amethyst_renderer/src/pass/util.rs, line 149 at r1 (raw file):

        None => return,
    };
    if let None = material {

Should be is_none; can be || together with the if-block below.

Comments from Reviewable
@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Feb 16, 2018

Member

Reviewed 6 of 13 files at r1.
Review status: 11 of 13 files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable
Member

torkleyy commented Feb 16, 2018

Reviewed 6 of 13 files at r1.
Review status: 11 of 13 files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 18, 2018

Member

Review status: 11 of 13 files reviewed at latest revision, 2 unresolved discussions.

amethyst_renderer/src/transparent.rs, line 79 at r1 (raw file):

Previously, Xaeroxe (Jacob Kiesel) wrote…

Should use a clear() and extend() here to re-use the allocation.

Done.

amethyst_renderer/src/pass/util.rs, line 149 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should be is_none; can be || together with the if-block below.

Done.

Comments from Reviewable
Member

Rhuagh commented Feb 18, 2018

Review status: 11 of 13 files reviewed at latest revision, 2 unresolved discussions.

amethyst_renderer/src/transparent.rs, line 79 at r1 (raw file):

Previously, Xaeroxe (Jacob Kiesel) wrote…

Should use a clear() and extend() here to re-use the allocation.

Done.

amethyst_renderer/src/pass/util.rs, line 149 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should be is_none; can be || together with the if-block below.

Done.

Comments from Reviewable
@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Feb 18, 2018

Member

:lgtm_strong:

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable
Member

torkleyy commented Feb 18, 2018

:lgtm_strong:

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable
@torkleyy

torkleyy approved these changes Feb 18, 2018
View changes

I like it :)

@jojolepro

jojolepro approved these changes Feb 18, 2018 • edited
View changes

lgtm

@Xaeroxe

Xaeroxe approved these changes Feb 18, 2018
View changes

LGTM!

bors r+

bors bot added a commit that referenced this pull request Feb 18, 2018

@bors[bot]
Merge #574 
574: feat: Support transparency in core passes r=Xaeroxe a=Rhuagh



<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/amethyst/amethyst/574)
<!-- Reviewable:end -->
Loading status checks…
53949e1
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot Feb 18, 2018

Contributor

Build succeeded
Contributor

bors bot commented Feb 18, 2018

Build succeeded

@bors bors bot merged commit 6127294 into amethyst:develop Feb 18, 2018
3 of 4 checks passed

3 of 4 checks passed

code-review/reviewable 1 discussion left (Xaeroxe)
Details
bors Build succeeded
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Rhuagh Rhuagh deleted the Rhuagh:feature/transparency branch Feb 18, 2018

@antoyo antoyo referenced this pull request Mar 17, 2018

Closed

Loaded sprites don't have transparencies #510

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