Reject fragment shader output locations > max_color_attachments limit
#8316
Conversation
…t `@builtin(position)` warning
|
Current CI failures are unrelated, and are resolved with #8317. |
ErichDonGubler
changed the title
locations > max_color_attachmentslocations > max_color_attachments
ErichDonGubler
added
area: api
ErichDonGubler
changed the title
locations > max_color_attachmentslocations > max_color_attachments
ErichDonGubler
changed the title
locations > max_color_attachmentslocations > max_color_attachments limit
ErichDonGubler
force-pushed
the
erichdongubler-push-boinging-erudite-lemon
branch
from
a8a62f6 to
5f9d9bc
Compare
| let &Varying::Local { location, ref iv } = output else { | ||
| continue; | ||
| }; | ||
| if location >= self.limits.max_color_attachments { |
There was a problem hiding this comment.
It is true today that this will always be equal to hal::MAX_COLOR_ATTACHMENTS (used for the error message), but I don't know if we want to rely on that.
There's also a question whether it is an error for an output that is not consumed by the pipeline to be placed at a location above maxColorAttachments. The spec describes maxColorAttachments as a limit on the number of entries in various arrays appearing in descriptors.
| | Self::NoEntryPointFound | ||
| | Self::MultipleEntryPointsFound => return ErrorType::Validation, | ||
| | Self::MultipleEntryPointsFound | ||
| | Self::ColorAttachmentLocationExceedsLimit { .. } => return ErrorType::Validation, |
There was a problem hiding this comment.
name bikeshedding: I would prefer ...TooLarge or ...OutOfBounds (strings that we already use in error variants) over ExceedsLimit. (Although I do see we have ExceededLimit... appearing in one place.)
I also might be inclined to make the name general enough to cover the case that the location lands on a gap between defined color targets (e.g. InvalidColorAttachmentLocation), although the primary responsibility for checking that seems to be later in create_render_pipeline (where it presently only generates a log::warn!), so maybe it's not important.
Found a spec. bug (see gpuweb/gpuweb#5341)! 😀 The WebGPU spec. didn't demand that we ensure that this was so, but it was intended to.
rebaseplz
Testing
TODO
Checklist
CHANGELOG.mdentry.