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

Ray Queries #6291

Merged
merged 374 commits into from
Nov 9, 2024
Merged

Ray Queries #6291

merged 374 commits into from
Nov 9, 2024

Conversation

Vecvec
Copy link
Contributor

@Vecvec Vecvec commented Sep 18, 2024

Connections
Works towards #1040

Description
This PR provides BLASes (bottom level acceleration structures), TLASes (top level acceleration structures), TLAS instances (which contain BLASes and data, like transforms, about them), TLAS packages (which contain vectors of TLAS instances).

Testing
Running tests & examples included in PR

This a updated version of #3631 and is intended to replace it.

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • n/a --target wasm32-unknown-unknown
    • n/a --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.
  • More tests & examples.
  • More docs

Later (follow-up PR)

  • Acceleration structure compaction.
  • Procedural geometry.
  • as_hal methods

@Vecvec
Copy link
Contributor Author

Vecvec commented Oct 19, 2024

I have noticed that quite a few resources use snatchables and it seems that they are used in resource destruction if the resource is in something such as a bind group, I will implement snatch guards for BLASes and TLASes while @cwfitzgerald works on moving the validation away from a mutable state on the BLAS/TLAS.

@Vecvec Vecvec mentioned this pull request Nov 3, 2024
@Vecvec
Copy link
Contributor Author

Vecvec commented Nov 8, 2024

@cwfitzgerald you were working on moving the validation away from a mutable state on the BLAS/TLAS, do you know how long that might take? It seems that other people are working off this branch (for example: #1040 (comment)), so it would be useful to get this branch into trunk.

@JMS55
Copy link
Contributor

JMS55 commented Nov 8, 2024

I coincidentally had just asked @cwfitzgerald about this on matrix. Posted their response below, with the first part paraphrased.

[explaining that they don't have time to work on this anymore]. However we don't actually want to block it at this stage so our resolution to this problem is to let it in basically as is. The one change that we want to make is prefix the feature flag with EXPERIMENTAL_ so that people will expect it to not work sometimes or break or break your computer [or] whatever.

So sounds like we can merge once you rename the feature flag :)

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Good after failing CI fixed

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) November 9, 2024 17:53
@cwfitzgerald cwfitzgerald merged commit 79a6f2c into gfx-rs:trunk Nov 9, 2024
27 checks passed
@JMS55
Copy link
Contributor

JMS55 commented Nov 9, 2024

Thanks so much @Vecvec! What a great birthday present ❤️

@teoxoy
Copy link
Member

teoxoy commented Nov 15, 2024

I noticed that this PR makes use of pending_writes but the operations adding things in pending writes are on the command encoder. Pending writes is only needed for queue operations that require a hal command encoder to be executed (ex write_buffer). I think the way the new command_encoder_build_acceleration_structures use pending writes is not sound or at least not what it was designed for. Shouldn't we add things to the command encoder itself rather than into pending writes?

@teoxoy
Copy link
Member

teoxoy commented Nov 15, 2024

Tlas.destroy don't seem to check if the Tlas is used in a bind group of an active submission. The only reason we need the destroy methods for textures and buffers is because they allow users to eagerly release memory in browser implementations. I think we can remove the destroy methods on the acceleration structures for now as they complicate the picture without any gain. If they will be needed for Firefox we can add them back.

@teoxoy
Copy link
Member

teoxoy commented Nov 15, 2024

I put up a PR with these changes ^. Could someone test it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants