Remove gpu-descriptor dependency by inlining the crate#9078
Remove gpu-descriptor dependency by inlining the crate#9078a1phyr wants to merge 11 commits intogfx-rs:trunkfrom
gpu-descriptor dependency by inlining the crate#9078Conversation
There was a problem hiding this comment.
So I'm no lawyer but if this is a line-for-line copy, that seems slightly legally dubious. More specifically, we will almost certainly need to bundle the copyright notice and license text, which it doesn't look like you did here. Overall, I'm not sure why we need this but I'll let another Mozillan handle it, since they have a whole system for these things.
Also, I realize this is an early review, and you might be in the middle of fixing the copyright fun business.
Also move the implementation to `descriptor.rs`.
This function is only ever used with a single item, so we can simplify it using this knowledge.
a1phyr
force-pushed
the
remove-gpu-descriptor
branch
from
ad76441 to
3ee67e4
Compare
|
Well, from the MIT license:
Which I did in |
|
On licensing, yeah this is fine, the one note would be to make the LICENSE field of wgpu-hal "(MIT OR Apache-2.0) AND MIT" to be fully formally correct. |
|
Well
|
inner-daemons
left a comment
inner-daemons
left a comment
There was a problem hiding this comment.
Preliminary review.
I'm curious, what's necessarily the goal here? Is there a reason we need to remove the dependency? And if we do remove the dependency, why not just rewrite the logic as needed? It seems like copying the code over left us with some unnecessary artifacts.
Anyway, thanks for the work put in here. I'm gonna probably just gonna defer to the next reviewer for most stuff.
| }; | ||
|
|
||
| if pool_size == Default::default() { | ||
| pool_size.sampler = 1; |
There was a problem hiding this comment.
I don't know. Maybe to prevent zero-sized pools?
wgpu-hal/src/vulkan/descriptor.rs
Outdated
| .max(minimal_set_count) // at least enough for allocation | ||
| .max(self.total.min(MAX_SETS)) // at least as much as was allocated so far capped to MAX_SETS | ||
| .checked_next_power_of_two() // rounded up to nearest 2^N | ||
| .unwrap_or(i32::MAX as u32); |
There was a problem hiding this comment.
This seems sketchy, plus shouldn't it be i32::MAX as u32 + 1 either way?
There was a problem hiding this comment.
Yes, probably (this branch was not reachable anyway). Replaced this code by a much more readable clamp. Not sure about what the next_power_of_two() is about though.
| .checked_next_power_of_two() // rounded up to nearest 2^N | ||
| .unwrap_or(i32::MAX as u32); | ||
|
|
||
| max_sets = (u32::MAX / self.size.sampler.max(1)).min(max_sets); |
There was a problem hiding this comment.
This definitely needs a comment explainign what its doing dividing by the descriptor count.
There was a problem hiding this comment.
Well probably, but I can't really explain it.
a1phyr
force-pushed
the
remove-gpu-descriptor
branch
from
1af5b40 to
7a6ac42
Compare
a1phyr
force-pushed
the
remove-gpu-descriptor
branch
from
7a6ac42 to
93a0882
Compare
Well, mainly the fact that it's not really maintained. Also this enables us to tailor fit this code.
This is what I started with some low-hanging fruits in commits after the first, but looks that I missed some. |
a1phyr
force-pushed
the
remove-gpu-descriptor
branch
from
93a0882 to
1a787ab
Compare
a1phyr
force-pushed
the
remove-gpu-descriptor
branch
from
1a787ab to
4b83123
Compare
|
CI failure is a DX12 timeout, so probably unrelated. |
4 participants
Description
Stop depending on
gpu-descriptorand inline the part of their code that we need directly.gpu-descriptoris very passively maintained and has outdated dependencies. Inlining the crate intowgpu-halprevents this and simplifies our code at a small cost (~300 LoC). Additionally, this should reduce compile times a bit.Each commit can be reviewed independently. The first commit bring parts of
gpu-descriptorandgpu-descriptor-typesinwgpu-hal/src/vulkan/descriptor.rsand does minor import changes to accommodate for this. Other are refactoring that take advantage of the inlining.Testing
Ran tests
Squash or Rebase?
Rebase
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.