pygfx/wgpu-py

Recent pull request has a bug

Closed this issue · 5 comments

fyellin commented

I made a mistake in a recent pull request. I will fix it this afternoon. (California time).

In _tuple_from_blend_component, I accidentally put the fields in the wrong order. I discovered this when I wrote actual code that elided these fields.

I will fix the bug and make sure it is adequately tested.

fyellin commented

I may want the advise of one of the more experienced developers in knowing the correct fix for this. I see where my confusion came from.

In the specification, the fields are in the order operation, srcFactor, dstFactor.

In wgpu-py, they are used in the order srcFactor, dstFactor, operation. See lines 1583-1953 in _api.py and line 269-271 of test_gui_glfw.py.

The specification also says nothing about allowing a tuple instead of a dictionary. This seems to be a wgpu-py innovation. In the other cases where we allow a tuple instead of a dictionary, this is explicitly allowed by the spec.

So should I be using (1) the order as given in the spec? (2) the order that wgpu-py seems to have settled on? or (3) only allow [a possibly empty] dictionary here and not allow a tuple or list at all.

almarklein commented

Thanks for the quick response to the found bug. And thanks for taking the time to compare our implementation with the spec. I think its important to follow the spec rather precisely, otherwise we may drift away from it over time.

The fact that the spec does not allow a tuple is a blessing; it means we don't have to choose between 1 and 2. So my answer would be to go with 3. It may break people's code who use tuples, but over time its worth it.

fyellin commented
  • 😄1
Korijn commented

This can be closed now right @fyellin ?

almarklein commented

The PR says Fix bug #532 but GH did not pick that up.