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

Solves issue #4562 #6699

Merged
merged 9 commits into from
Jan 14, 2024
Merged

Solves issue #4562 #6699

merged 9 commits into from
Jan 14, 2024

Conversation

Garima3110
Copy link
Member

Solves issue #4562
Changes:
I have introduced a separate variable for pre-erase blend mode, which can maintain clarity in the code and avoid conflicts between the optimization goal and erasing logic. I hope this should make the blend state management more straightforward and easier to maintain.
I would love to have the suggestions of the maintainers to this solution, thanks a lot!

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this on! It looks like npm run test is currently failing, so it seems like right now this accidentally changes behaviour. That might just be from a potential typo in the code that I commented on, could you see if that fixes it or if there are still test failures?

@@ -1226,7 +1226,7 @@ p5.RendererGL.prototype._applyColorBlend = function (colors) {
* @return {Number[]]} Normalized numbers array
*/
p5.RendererGL.prototype._applyBlendMode = function () {
if (this._cachedBlendMode === this.curBlendMode) {
if (this._curBlendMode === this.preEraseBlend) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be curBlendMode without the underscore?

@Garima3110
Copy link
Member Author

Thanks for taking this on! It looks like npm run test is currently failing, so it seems like right now this accidentally changes behaviour. That might just be from a potential typo in the code that I commented on, could you see if that fixes it or if there are still test failures?

Thanks a lot @davepagurek for looking into this!
The tests still fail, I am trying to find out the exact cause , If you have any suggestions please do let me know.! Thanks:-)

@davepagurek
Copy link
Contributor

The error output says that this test is failing, in test/unit/color/setting.js line 94:

test('should cache renderer blend', function() {
  my3D.blendMode(my3D.SCREEN);
  my3D.erase();
  assert.deepEqual(my3D._renderer._cachedBlendMode, my3D.SCREEN);
});

I think this failure is just because you renamed that property, so I believe you'll need to update that test to refer to it under its new name.

@Garima3110
Copy link
Member Author

I think this failure is just because you renamed that property, so I believe you'll need to update that test to refer to it under its new name.

I have made the changes. Please have a look and let me know if some more changes are to be made . Thanku!

*/
p5.RendererGL.prototype._applyBlendMode = function () {
if (this._cachedBlendMode === this.curBlendMode) {
if (this.curBlendMode === this.preEraseBlend) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking this through a bit more, I think this might be slightly mixing the logic a bit. I think this method's intention is to only change the blend settings if the mode has changed from what we were last using, regardless of whether or not that was used for erasing. So I believe we need two stored properties: a preEraseBlend like you have, but also a cachedBlendMode, referring to the last value that _applyBlendMode was called with. So everything in this method would be using a cachedBlendMode, and the erase/noErase ones would refer to preEraseBlend.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @davepagurek for the insights. I'll just look into this and get back to you soon ..!

@Garima3110
Copy link
Member Author

I've made the suggested changes . Please have a look!

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think this looks good now!

@davepagurek davepagurek merged commit f4cc7e1 into processing:main Jan 14, 2024
2 checks passed
@Garima3110
Copy link
Member Author

Thanks, I think this looks good now!

Thanks @davepagurek for your help on this . Got more insights on the codebase. All thanks to you..! :-)

@Garima3110 Garima3110 deleted the blend-state branch January 14, 2024 17:33
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