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

Fixed ability to use all the drawing modes #1809

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

skibu
Copy link
Contributor

@skibu skibu commented Jan 27, 2025

Turns out that BLEND_MODES was missing ['SATURATE'] = 3 which was preventing one from using that draw mode and any of the draw modes after that one. Quite limiting! I'm certain this was just a typo.

Since added a BLEND_MODE also had to fix screen_set_operator().

skibu added 2 commits January 27, 2025 14:27
Added the missing ['SATURATE'] = 3, and then shifted the numbers for all the subsequent values.
@tlubke
Copy link
Collaborator

tlubke commented Jan 28, 2025

(#1247 (comment)) Based on this, I think the index limit and omission of the SATURATE mode were purposeful at the time.

Here's all of the reasoning that I can remember.

  • ADD output is identical to SATURATE (I can't remember if this is because of being grayscale).
  • DEST_ATOP should be identical to OVER (which is the default operator mode).
  • HSL_HUE, HSL_SATURATION, HSL_COLOR, and HSL_LUMINOSITY don't provide much value in grayscale.

Since screen.set_operator() can take either a string or a number, changing the index order could break existing scripts that might use it with an index directly.


Having said all that, the range-check change in screen.c looks correct, and it may be worth considering removing the limit for the purpose of interacting with non-grayscale colorspaces through mods.

@skibu
Copy link
Contributor Author

skibu commented Jan 28, 2025

Ah, SATURATE duplicating the functionality of ADD explains why it was removed in screen.lua . But there is still a definite bug that causes the operators beyond ADD to not work correctly because the indexes are off by one (even if one uses the name of the operator instead of the number). I tried both using the index and the name of the operator and I got incredibly confusing results since the wrong operator was being used.

The root of the problem is a mismatch between the operator list in screen.lua and in screen.c .

In screen.c the list of operators is:

static cairo_operator_t ops[NUM_OPS] = {
    CAIRO_OPERATOR_OVER,
    CAIRO_OPERATOR_XOR,
    CAIRO_OPERATOR_ADD,
    CAIRO_OPERATOR_SATURATE,
    CAIRO_OPERATOR_MULTIPLY,
    CAIRO_OPERATOR_SCREEN,
   ...

If SATURATE should indeed not exist (will need you to confirm this) then proper solution would be to also update the operator list in screen.c so it matches the list in screen.lua .

@tlubke
Copy link
Collaborator

tlubke commented Jan 28, 2025

Ah, I see the issue now. Yes, I think the indices in screen.lua are the intended ones, and screen.c was not updated appropriately. It doesn't look this was a regression either (I'm surprised it took nearly 5 years for the issue to be detected!).

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.

2 participants