Implement push-constants

[fyellin]

Initial implementation for push_constants.

Since the only other implementations of this are in C-like interfaces, there are some open API questions.

1. In the binding code, we use start/end while in set_push_constant we use start/length. This is from the underlying API. Should we keep this?

2. Do we want to add a data_offset argument to set_push_constants()? It's not in the Rust API, but might be useful.

In general, any of the added functions or functionality could easily have their APIs better designed for consistency.

[almarklein]

As for the API, we follow WebGPU, only deviating when necessary.

Looks like push constants are part of the ProgrammableStage dict that's embedded in the args passed to create_render_pipeline()

From https://gpuweb.github.io/gpuweb/#gpuprogrammablestage:

That constants must be a dict mapping constant name to constant value (A GPUPipelineConstantValue is really just a double).

[fyellin]

@almarklein :

That's a different feature, but with a similar name. It lets you create variables that are "almost constant" but can be changed at pipeline creation time. It's actually part of the webgpu spec, but not implemented in the current wgpu.

https://gpuweb.github.io/gpuweb/wgsl/#override-decls

[fyellin]

The API for createPipelineLayout was inspired by Rust's PipelineLayoutDescriptor which has two public fields, bind_group_layouts and push_constant_ranges. I let the fields of the dictionary match the fields of the wgpu.h API, except that stages become visibility in line with other locations.

The API for set_push_constants is precisely equivalent to that in wgpu.h. I think there needs to be an optional data_offset field added to make this code similar to write_buffer.

I'm a little bit bothered that limits are snake_case_with_underscores while features are snake-case-with-hyphen. This makes the code for adapter.get_device() look a little bit strange. I went ahead and fixed this. Let me know if I shouldn't have done this.

[almarklein]

That's a different feature, but with a similar name.

Ah, both of these are new to me, so I mixed them up.

If I understand correctly, push constants are a feature specific to wgpu-native, not present in WebGPU. In that case, the API should not be in _classes.py, because that api should be mappable to any WebGPU(ish) backend. So far, we've implemented wgpu-native-specific features in backends/wgpu_native/extras.py, which get exposed in the wgpu.backends.wgpu_native namespace. That way it is clear when code uses 'special' functionality and might therefore be less portable.

[almarklein]

Nice work on the limits and features! I agree that we should use a consistent naming scheme.

For compatibility, are both dash-case and snake_case allowed when setting features/limits? Maybe have a test for this to keep it that way.

[fyellin]

Code should now accept my_feature, my-feature, and myFeature for limit and feature names.
Still need to work on doc.rst, but everything else should be done.
_classes.py should be unchanged by this CL, since all changes are wgpu-native specific.

[fyellin]

Don't review yet. Premature. _api.py is going to be a painful merge. Working on it.

[fyellin]

Merge is complete and is ready to be reviewed (but not quite ready for submission).

When I run pytest tests_mem on my Mac, everything works fine. But the test fails as part of the test suite on Linux with

----------------------------- Captured stdout call ----------------------------- more after create: {'RenderBundle': (32, 32), 'RenderBundleEncoder': (32, 0)} =========================== short test summary info ============================ FAILED tests_mem/test_objects.py::test_release_render_bundle - AssertionError: Expected: {'RenderBundle': (32, 32)} Got: {'RenderBundle': (32, 32), 'RenderBundleEncoder': (32, 0)} =================== 1 failed, 28 passed, 2 skipped in 8.11s ====================

I've no idea why I'm getting different results on the two. I also confess I don't understand what @test_and_release is testing and what the two numbers are supposed to represent. The decorator has no documentation saying what its arguments mean.

[fyellin]

This is ready for review.
I'm not completely happy with test_objects(), but both both branches "main" and this branch both fail on my Mac. Apparently there is a slight difference in the implementation of render bundles.

@almarklein: I do wish it were better documented what @create_and_release was testing, and what the two numbers are. Python objects and native objects?

[almarklein]

@almarklein: I do wish it were better documented what @create_and_release was testing, and what the two numbers are. Python objects and native objects?

Sorry for the missing docs here. Yeah, they are Python and native (Rust) objects. Would you mind adding that to the docstring in this pr?

[almarklein]

I'm not completely happy with test_objects(), but both both branches "main" and this branch both fail on my Mac. Apparently there is a slight difference in the implementation of render bundles.

Is that resolved? I don't see a change in test_objects() now.

[fyellin]

The test_objects problem was resolved. A rechecking of _api.py indicated that I had missed a tiny change where self was replaced with self._device. I will look at other comments this afternoon. Sent from Gmail Mobile

…

On Fri, Sep 13, 2024 at 10:04 Almar Klein ***@***.***> wrote: I'm not completely happy with test_objects(), but both both branches "main" and this branch both fail on my Mac. Apparently there is a slight difference in the implementation of render bundles. Is that resolved? I don't see a change in test_objects() now. — Reply to this email directly, view it on GitHub <#574 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABC2LILDD7VROPGB3JFCNQLZWKE7BAVCNFSM6AAAAABN4UWWZ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBYGE4DMMBQGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[almarklein]

Only a few typos and adding some extra context in the memtest docs.

[almarklein]

Thanks!