Removed hardcoded thickness clamp in DrawNode

[w1257]

Describe your changes

• Removed the thickness clamp in DrawNode::_drawSegment to allow greater control. Useful when DrawNode scale>1.0f and line 1px<thickness<1.0f.
• Updated drawLine to clarify thickness behavior and relation to the drawOrder.
• Added myself to the AUTHORS.md .
• Tested on Windows 11 with DrawNode::DrawLine.

Issue ticket number and link

Checklist before requesting a review

For each PR

• Add Copyright if it missed:
  - "Copyright (c) 2019-present Axmol Engine contributors (see AUTHORS.md)."

• I have performed a self-review of my code.

  Optional:

  • I have checked readme and add important infos to this PR.
  • I have added/adapted some tests too.

For core/new feature PR

• I have checked readme and add important infos to this PR.
• I have added thorough tests.

Axmol 3.x ------------------------------------------------------------

For each 3.x PR

• Check the '#include "axmol.h"' and replace it with the needed headers.

[aismann]

Useful when DrawNode scale>1.0f and line 1px<thickness<1.0f

Can you add a cpp-test which "shows/check" for future changes/checks too?

scale: >1.0f, ==1.0f, <1.0f
thickness >1.0f, == 1.0f, <1.0f

You know there is a methode setting the scale too?

    /** Set the DrawNode scale for each drawing primitive after this.

         drawNode->properties.setScale(myScaleNew);
         [... draw what ever you want with the new scale ... ]
         drawNode->properties.setScale(myScaleBefore);

[w1257]

I will add the cpp-test for it using DrawNode::DrawLine.

I did not know about properties.setScale until now but it doesn't seem to be actually relevant to the drawLine (checked the code and the game with it, it seems to just apply scale on top of node scale but differently), I was referring to the node scale which is quite relevant when for example in 1 tile : 1 unit in grid based setup where the parent node is scaled up to achieve that. I also did not know about properties.setFactor which is relevant here which can achieve same results.

I will also update the DrawNode class comment documentation to point at the properties.

[aismann]

I will add the cpp-test for it using DrawNode::DrawLine.

Thanks. Please add also properties.setFactor to the new test.

I will also update the DrawNode class comment documentation to point at the properties.

Thank you. That's really helpful.

[aismann]

I run one of the builds "windows_ogl_x64":
thickness <1.0 (not all lines will be "shown") Thats was the reason for the hardcoded clamp:

thickness 1.0: all lines (colors) shown

[w1257]

The clamp does not consider the scaling.

I am still investigating the code looking for the potential options but I think it would be easier and better to push that responsibility to developer to take care of sub-pixel real thickness themself. The safe default is there already there and it works quite consistently without the clamp.

Current clamp does not work here. 1.5(thickness)*0.5(scale, default value in the test)=0.75(theoretical real thickness)=potencial sub-pixel. Resolution here: 1327x672 (there are more of them).

Also adding proper check would likely involve bit more complex stuff (such as considering both scales and factor) which might add overhead on what I am guessing minority of cases while slowing down the majority. If developer has issues, they got all the tools (and documentation soon) to deal with it.

[aismann]

Also adding proper check would likely involve bit more complex stuff (such as considering both scales and factor) which might add overhead on what I am guessing minority of cases while slowing down the majority. If developer has issues, they got all the tools (and documentation soon) to deal with it.

Thanks for investigating.
I will fix the wrong behavior of thickness >1.0f <2.0f

@w1257
Want you add a cpp-test test for your usecase/situation?

[w1257]

I will make the test testing both scales, factor and thickness. I am planning to make a hard test that will make sure that maths checks out when making changes. I will use images and drawing on each other which should be visually clear state of the test and potentially it can be fully automated in the future. There will be also free form test included.

If you want, I can make the hard test image consistently generative (Not AI gen, custom gen code. It will take less space in the repo but will take more time to load). Or image file(s) (takes more space but faster to load.)

Making the hard tests will most likely take more time due to my availability. I will make them last.

[aismann]

Making the hard tests will most likely take more time due to my availability. I will make them last.

Fine.

I will improve the DrawNode performence tests (add sliders for factor and scale).

[w1257]

finalPixelWidth = 2 * finalPixelRadius = 2 * node.scale * (thickness / (2 * properties.factor)); // _drawSegment

or

finalPixelWidth = 2 * finalPixelRadius = 2 * node.scale * (thickness /  properties.factor); // _drawPolygon

The formula is wrong I think.
The reference is the thickness of each drawing primitive with the default settings of the properties

So the hardcoded factor (2) can be different on each drawing primitive (line, circle, polygon, etc)

[aismann]

@w1257
It is more of a design decision as to what the DrawNode properties should do and how they should work:
Cocos2dX 4.0 did not have this feature.
Here is the design desicion of Axmol's class DrawNode::properties:

// Create a DrawNode object
        auto drawNode = DrawNode ::create();

// The Node scale works for the entire node.
// This is not always useful and should be improved
         
        drawNode->setScale(2);                         // Node behavior (scale all)
        
// The properties affect all subsequent drawNode->draw* methods as long as properties.transform = true.
// Performance: Since there are “not that many use cases for this behavior" it must be enabled.

    // Works without setTransform(true)
    drawNode->properties.setFactor(...);              // only thickness 

[ draw what ever you want => with default properties settings ]

        drawNode->properties.setTransform(true);   // activate/enable properties 

        drawNode->properties.setScale(Vec2(2,2));  ;    // only position
        drawNode->properties.setScale(Vec2(2,2), scaleThickness); ; // position + thickness

[ draw what ever you want with the changed properties ]

        drawNode->properties.setTransform(false);   // deactivate/disable properties 
        
// set the "default" properties values

        drawNode->properties.setDefaultValues();

[ draw what ever you want => with default properties settings ]

HINT:

drawNode->properties.setScale(Vec2(2,2), scaleThickness); ; // position + thickness

Was not implemented. You can reach the same behavior using:

        drawNode->properties.setFactor(2);              // only thickness 
        drawNode->properties.setScale(Vec2(2,2));   // only position

Last but not least:
position means "resizing of the positions"

[aismann]

@w1257
I think I have the right formula but I have to test it some more
Have to make more tests before send a PR.
Thanks again for your patient.

[aismann]

@w1257
please use fix: #3076

And please add your test. Thanks.
I removed the draft flag too.

[halx99]

@aismann does #3076 supersede this PR? If so, we can close this PR.

[aismann]

@aismann does #3076 supersede this PR? If so, we can close this PR.

@halx99
I think we can close it.

w1257 has planned his own DrawNode-cpp tests, maybe some documentation as well.

@w1257
Please open a new PR that better describes your upcoming changes/improvements.

[w1257]

I will create a new PR for the documentation update and new tests.