/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

refactor: Make passes work on Visibility rather than Transparent directly #595

Merged
merged 1 commit into from Mar 9, 2018

Conversation

Reviewers

@jojolepro jojolepro

@torkleyy torkleyy

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

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

This change is Reviewable
@jojolepro

jojolepro requested changes Mar 6, 2018
View changes

A good step towards optimizing that renderer :)

amethyst_renderer/src/pass/flat/interleaved.rs
- for entity in &back_to_front.entities {
- if let Some(mesh) = mesh.get(*entity) {
+ match visibility {
+ None => for (mesh, material, global) in (&mesh, &material, &global).join() {

This comment has been minimized.

Show comment
Hide comment
@jojolepro

jojolepro Mar 6, 2018

Collaborator

Format difference with line 126.
@jojolepro

jojolepro Mar 6, 2018

Collaborator

Format difference with line 126.
amethyst_renderer/src/pass/flat/separate.rs
- for entity in &back_to_front.entities {
- if let Some(mesh) = mesh.get(*entity) {
+ match visibility {
+ None => for (entity, mesh, material, global) in

This comment has been minimized.

Show comment
Hide comment
@jojolepro

jojolepro Mar 6, 2018

Collaborator

format difference with line 152
@jojolepro

jojolepro Mar 6, 2018

Collaborator

format difference with line 152
amethyst_renderer/src/pass/pbm/interleaved.rs
- for entity in &back_to_front.entities {
- if let Some(mesh) = mesh.get(*entity) {
+ match visibility {
+ None => for (mesh, material, global) in (&mesh, &material, &global).join() {

This comment has been minimized.

Show comment
Hide comment
@jojolepro

jojolepro Mar 6, 2018

Collaborator

that line is formatted wrong in all passes basically
@jojolepro

jojolepro Mar 6, 2018

Collaborator

that line is formatted wrong in all passes basically

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Mar 6, 2018

Member

Nope, that's what rustfmt wants it to be, since it's a single block.
@Rhuagh

Rhuagh Mar 6, 2018

Member

Nope, that's what rustfmt wants it to be, since it's a single block.
amethyst_renderer/src/visibility.rs
+ camera_distance: centroid.distance2(camera_centroid),
+ from_camera: centroid - camera_centroid,
+ })
+ .filter(|c| c.from_camera.dot(camera_forward) > 0.), // filter entities behind the camera

This comment has been minimized.

Show comment
Hide comment
@jojolepro

jojolepro Mar 6, 2018

Collaborator

Doesn't take into account the camera FOV.
If its > 180, you can see behind the camera.
If its < 180, you might not be culling things that could be culled.

If the center of the object is behind the camera, but stretches towards the front of it, it won't be shown and will pop in and out of existence.
@jojolepro

jojolepro Mar 6, 2018 •
edited
Edited 1 time
  • @jojolepro jojolepro edited Mar 6, 2018 (most recent)

Collaborator

Doesn't take into account the camera FOV.
If its > 180, you can see behind the camera.
If its < 180, you might not be culling things that could be culled.

If the center of the object is behind the camera, but stretches towards the front of it, it won't be shown and will pop in and out of existence.

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Mar 6, 2018

Member

This is known, to handle all those cases requires a much larger amount of math, which basically requires bounding volumes etc, which I don't want to add here. This is a system for fast prototyping, and will with 99% likelyhood not be used in larger games.
@Rhuagh

Rhuagh Mar 6, 2018

Member

This is known, to handle all those cases requires a much larger amount of math, which basically requires bounding volumes etc, which I don't want to add here. This is a system for fast prototyping, and will with 99% likelyhood not be used in larger games.

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Mar 6, 2018

Member

Note that the Visibility structure can handle those since it has no concept of all that, it's up to the culling system to do all those things. Also note that amethyst-rhusics is likely to have a better implementation for this, since there we can do real frustum culling based on the bounding volumes.
@Rhuagh

Rhuagh Mar 6, 2018 •
edited
Edited 1 time
  • @Rhuagh Rhuagh edited Mar 6, 2018 (most recent)

Member

Note that the Visibility structure can handle those since it has no concept of all that, it's up to the culling system to do all those things. Also note that amethyst-rhusics is likely to have a better implementation for this, since there we can do real frustum culling based on the bounding volumes.
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Mar 8, 2018

Member

r? @torkleyy @Xaeroxe @omni-viral
Member

Rhuagh commented Mar 8, 2018

r? @torkleyy @Xaeroxe @omni-viral
@torkleyy

torkleyy approved these changes Mar 8, 2018
View changes

Nice 👍

amethyst_renderer/src/pass/pbm/interleaved.rs
+ false,
+ mesh_storage.get(mesh),
+ None,
+ &*tex_storage,

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Mar 8, 2018

Member

Are these derefs necessary? I don't think so.
@torkleyy

torkleyy Mar 8, 2018 •
edited
Edited 1 time
  • @torkleyy torkleyy edited Mar 8, 2018 (most recent)

Member

Are these derefs necessary? I don't think so.

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Mar 8, 2018

Member

I couldn't find a different way without sending Fetch etc
@Rhuagh

Rhuagh Mar 8, 2018

Member

I couldn't find a different way without sending Fetch etc

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Mar 8, 2018

Member

I'm an idiot, nevermind me. It's been fixed.
@Rhuagh

Rhuagh Mar 8, 2018

Member

I'm an idiot, nevermind me. It's been fixed.
@torkleyy

torkleyy approved these changes Mar 9, 2018
View changes

Thank you!

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Mar 9, 2018

Member

Bors r=torkleyy
Member

Rhuagh commented Mar 9, 2018

Bors r=torkleyy

bors bot added a commit that referenced this pull request Mar 9, 2018

@bors[bot]
Merge #595 
595: refactor: Make passes work on Visibility rather than Transparent directly r=torkleyy 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/595)
<!-- Reviewable:end -->
Loading status checks…
ca8438b
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot Mar 9, 2018

Contributor

Build succeeded
Contributor

bors bot commented Mar 9, 2018

Build succeeded

@bors bors bot merged commit b1bb2a4 into amethyst:develop Mar 9, 2018
3 checks passed

3 checks passed

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

@Rhuagh Rhuagh deleted the Rhuagh:refactor/transparent-to-visibility branch Mar 9, 2018

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