Visible artifacts on DrawNode::drawCircle() with a corresponding lineWidth > 5 (window resizing issue)

[aismann]

THIS PROJECT IS IN DEVELOPMENT MODE. Any pull requests should merge to branch dev, otherwise will be rejected immediately.

Describe your changes

[Fix DrawNode::drawCircle() behavior itf the window is resizing.]
I tested it with cpp-test again => no effects on other nodes, objects or other stuff.

Issue ticket number and link

#1046

Checklist before requesting a review

• I have performed a self-review of my code.
• If it is a core feature, I have added thorough tests.
• I have checked readme and add important infos to this PR (if it needed).
• I have added/adapted some tests too.

[aismann]

Hope all questions be closed

[halx99]

Let @rh101 help us.

[aismann]

Let @rh101 help us.

Good idea.
If the thick allways smaller as the red api rendering going bad so the green api is never needed ;)

Look on the result of the "red" API if the thickness >10 or more. Thats the reason implementing the green API. (And I want not write a new Api because the drawCircle is connected with the global thicknees. Of course developer can use always a new DrawNode object but this make the rendering slower.

[rh101]

I'm having a lot of trouble following what this is all about.

Can you please summarize, concisely, what the purpose of this change is?

If the thick allways smaller as the red api rendering going bad so the green api is never needed ;)

Look on the result of the "red" API if the thickness >10 or more. Thats the reason implementing the green API. (And I want not write a new Api because the drawCircle is connected with the global thicknees. Of course developer can use always a new DrawNode object but this make the rendering slower.

This is confusing. Are you stating that we need to use one API if thickness is less or equal to 10, and a different API for thickness >10?

[aismann]

I will answer later

[aismann]

please look here: #1046

[rh101]

@aismann A few questions related to your changes:

1 - Will your changes result in a correct representation of a circle being drawn (at the correct scale), with no visible artifacts, regardless of line thickness?

2 - If the answer to the previous question is "Yes", then is the cause of all the confusion in this thread related to this specific test implementation?

Now that I'm looking at this specific issue, I do see a problem, which is that the API may also be an issue. It may have been better to leave drawCircle as it was previously (without the minThickness parameter), and a new method with a different name added for the optimized circle drawing (like drawCircleOptimized etc). The minThickness parameter also doesn't mean what a developer may think it means without reading the comments in the code, so a better name for that would be beneficial to reflect its true purpose.

[aismann]

1 - Will your changes result in a correct representation of a circle being drawn (at the correct scale), with no visible artifacts, regardless of line thickness?

Yes, but its not so fast as the 'original' drawCircle() method which works good if the thickness <5...8.

2 - If the answer to the previous question is "Yes", then is the cause of all the confusion in this thread related to this specific test implementation?

Yes and No. This PR is optimizing the situation if the window is resizing.
Yes:
The visual comparing is the reason for so much "confusion". If you comment the 'original' drawCircle() line the 'understand will be better and its having not so much questions here ;)
No:
This PR is optimizing the situation if the window is resizing. And one of the goal is having the same "thickness" as the original drawCircle() method. That's the reason to show 'both' drawCircle methods on the same scene (for comparing the same behavior, but maybe this is not so important as I thought).

Now that I'm looking at this specific issue, I do see a problem, which is that the API may also be an issue. It may have been better to leave drawCircle as it was previously (without the minThickness parameter), and a new method with a different name added for the optimized circle drawing (like drawCircleOptimized etc). The minThickness parameter also doesn't mean what a developer may think it means without reading the comments in the code, so a better name for that would be beneficial to reflect its true purpose.

I think having a drawCircleOptimized method developers will write source code like this:

if (_lineWidth < 5) 
    drawCircle(...);
else
    drawCircleOptimized(...);

That's the same as using
drawCircle(... , /*minThickness */ 5);

Of course the minThickness name can be missunderstanding but this example above is helping to understand what it mean
The tester helps too: Shows how its works.

And last but not least: minThickness name can be changed also.
I think cocos2dx/axmol having more such APIs which need to read the 'API description' for a better understanding what it is doing.

and
drawCircle(... , /*minThickness */ 0);
is the drawCircleOptimized method (optimized means 'rounder' not faster).

[rh101]

I understand what you mean now, and yes, it may be that a lot of the confusion is coming from that specific test.

Now, related to the API, you make a fair point regarding the optimization in that method, and what do you think about changing minThickness to minOptimizedLineWidth or something similar would make more sense?

[aismann]

I understand what you mean now, and yes, it may be that a lot of the confusion is coming from that specific test.

I will make the tester more non confusion with a later PR

Now, related to the API, you make a fair point regarding the optimization in that method, and what do you think about changing minThickness to minOptimizedLineWidth or something similar would make more sense?

I think threshold is the best name for this 'optional' parameter.

[rh101]

I think threshold is the best name for this 'optional' parameter.

That's actually a good idea. Perhaps you can add more to it so to give it some context, such as "optimizationThreshold" etc, which would indicate what that parameter is being used for.

[aismann]

Now, related to the API, you make a fair point regarding the optimization in that method, and what do you think about changing minThickness to minOptimizedLineWidth or something similar would make more sense?

I will write some more about this parameter on the APPENDIX.md
@halx99
Plz merge this PR (because its looks better as the current implementation of this 'feature')
I start a new PR with all ideas and hints (thanks @rh101 @halx99 ) some days later.

[halx99]

The scale code still confuse me:

 auto borderScale = (glView->getScaleX() + glView->getScaleY());
drawPolygon(vertices, segments, Color4B(1.0f, 0.0f, 0.0f, 1.0f), _lineWidth / borderScale , color);

If glView scale smaller then the borderWidth will be larger?

[halx99]

The scale code still confuse me:

 auto borderScale = (glView->getScaleX() + glView->getScaleY());
drawPolygon(vertices, segments, Color4B(1.0f, 0.0f, 0.0f, 1.0f), _lineWidth / borderScale , color);

If glView scale smaller then the borderWidth will be larger?

My suggestion is don't relay on glView scale just use follow code:

drawPolygon(vertices, segments, Color4B(1.0f, 0.0f, 0.0f, 1.0f), _lineWidth, color);

[aismann]

OK. I make all changes first.

[aismann]

The scale code still confuse me:

 auto borderScale = (glView->getScaleX() + glView->getScaleY());
drawPolygon(vertices, segments, Color4B(1.0f, 0.0f, 0.0f, 1.0f), _lineWidth / borderScale , color);

If glView scale smaller then the borderWidth will be larger?

My suggestion is don't relay on glView scale just use follow code:

drawPolygon(vertices, segments, Color4B(1.0f, 0.0f, 0.0f, 1.0f), _lineWidth, color);

Yes i will change the Line...no longer need to have the same bevior as the old api.

[aismann]

I have no right access :( to solve the conflicts

[aismann]

@halx99
For me its finished.
Plz merge it if it ok for axmol