Skip to content

Conversation

@GKFX
Copy link
Contributor

@GKFX GKFX commented Dec 14, 2016

calc*** color variables deleted; colorCalc now returns a float[] {r,g,b,a} array, with values from 0 to 1, like the old calc[RGBA] variables. The gray color methods don't bother with colorCalc at all.
The fill and stroke methods were also non thread-safe and values could easily pass between them.
I ran a number of tests with random inputs to the new color functions and they differed by one (eg. ffE5E5E5ffE6E6E6) from the previous version in a tiny number (seven in 10 million) of cases due to different rounding but were otherwise identical.
Fixes #3077.

calcEtc color variables deleted; colorCalc now returns a float[4] array.
@benfry
Copy link
Contributor

benfry commented Dec 15, 2016

This runs counter to an intentional design decision in the code because it would mean a lot of allocation and garbage collection for that array as it's re-created and destroyed inside tight draw() loops. You'll notice that all the low-level functions in core are designed to avoid allocation wherever possible because they can lead to stuttering. A proper solution for this issue needs to minimize the use of new for a bunch of temporary variables.

@benfry benfry closed this Dec 15, 2016
@GKFX
Copy link
Contributor Author

GKFX commented Dec 15, 2016

@benfry That's fine; I'd like to replace the floats fillR etc with a final float[] fillColor that can be passed to calcColor, which would also return the color as an int in 0xAARRGGBB form, so that the color() functions would not need to pass the float[] array. Should I delete fillR etc., tag them depreciated but keep filling them, or leave them alone entirely? The same applies to stroke and others.
In other words I don't know whether third party code is expected to use protected methods/variables or whether it's okay to change them like this.

@benfry
Copy link
Contributor

benfry commented Dec 17, 2016

Messing with fillR would break all renderer subclasses (OpenGL in particular). The solution would need to work across all renderers and not break existing code.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

color() function is not thread-safe

2 participants