Skip to content

DrawNode local properties full supported now, removed all hardcoded clamps; DrawNode cpp-tests switched to ImGUI ; #3076

Merged
halx99 merged 39 commits intoaxmolengine:devfrom
aismann:DrawNode_properties_full_support_remove_clamps
Mar 12, 2026
Merged

DrawNode local properties full supported now, removed all hardcoded clamps; DrawNode cpp-tests switched to ImGUI ; #3076
halx99 merged 39 commits intoaxmolengine:devfrom
aismann:DrawNode_properties_full_support_remove_clamps

Conversation

@aismann
Copy link
Contributor

@aismann aismann commented Mar 2, 2026

Describe your changes

DrawNode::properties full_support + all hardcoded clamps removed
_drawTriangles removed (= part of _drawPoly or _drawPolygon)

=> Redesign: "flat Properties members"

  • removed all "hardcoded clamps"
  • add const to all properties getters
  • add missing DrawNode::properties.getScale()
  • removed pragma push_macro("TRANSPARENT") section
  • cpp-tests DrawNode: switch from ax::ui to ax::extension::ImGUI
  • class ax::Label adapt properties: underline/strikethrough

  • check/adapt axmol\ui (axmol\ui\UIMediaPlayer.cpp:
  • check/adapt AX_LABEL_DEBUG_DRAW
  • check/adapt AX_SPRITE_DEBUG_DRAW
  • check/adapt AX_SPRITEBATCHNODE_DEBUG_DRAW (not implemented)
  • check/adapt AX_LABELBMFONT_DEBUG_DRAW
  • check/adapt AX_LABELATLAS_DEBUG_DRAW (not implemented)
  • check/adapt FairyGUI (extensions\fairygui\src\fairygui\GGraph.cpp)
  • check/adapt Spine (extensions\spine\src\spine\SkeletonRenderer.cpp)
  • check/adapt templates\cpp\Source\MainScene.cpp
  • check/adapt tests\live2d-tests\Source\SampleScene.cpp
  • check/adapt cpp-tests:(ActionsTest/BugsTest/ClippingNodeTest/MultiTouchTest/PhysicsTest/RendererTest/SpritePolygonTest/TileMapTest/UIListViewTest)

see also diffs

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 aismann marked this pull request as draft March 2, 2026 10:03
@aismann
Copy link
Contributor Author

aismann commented Mar 2, 2026

@w1257 please give feedback too (= optional reviewer)

@aismann
Copy link
Contributor Author

aismann commented Mar 2, 2026

Adapt Label.cpp underline/strikethrough thickness:
image

Copy link

@w1257 w1257 left a comment

Choose a reason for hiding this comment

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

For the future PR(s), I suggest moving some of the sliders to the ImGUI inspector.

@aismann
Copy link
Contributor Author

aismann commented Mar 5, 2026

For the future PR(s), I suggest moving some of the sliders to the ImGUI inspector.

Good idea!
But switching between the inspector and the sliders on the scene is strange.
I think about it.

I like the idea, add an own Dialog:
image

@aismann
Copy link
Contributor Author

aismann commented Mar 6, 2026

@halx99
can you have a look plz?

[ 40%] Built target plainlua
gmake[1]: *** [CMakeFiles/Makefile2:3637: tests/lua-tests/CMakeFiles/lua-tests.dir/rule] Error 2
gmake: *** [Makefile:879: lua-tests] Error 2
build.ps1: /home/runner/work/axmol/axmol/tools/cmdline/axmol.ps1:89
Line |
  89 |          . $build_script @sub_args
     |          ~~~~~~~~~~~~~~~~~~~~~~~~~
     | Build fail, ret=2
Error: Process completed with exit code 1.

@aismann aismann requested review from halx99 and rh101 March 7, 2026 14:16
@rh101
Copy link
Contributor

rh101 commented Mar 7, 2026

For the future PR(s), I suggest moving some of the sliders to the ImGUI inspector.

Good idea! But switching between the inspector and the sliders on the scene is strange. I think about it.

ImGUI is not implemented for all platforms (such as iOS), so using ImGUI to control aspects of test cases would cause issues if running the cpp-tests project on such platforms.

@aismann
Copy link
Contributor Author

aismann commented Mar 11, 2026

@halx99
From my point of view, ready for review again.

@aismann aismann marked this pull request as ready for review March 11, 2026 11:21
halx99 added 2 commits March 11, 2026 21:21
Co-authored-by: halx99 <halx99@live.com>
@halx99
Copy link
Collaborator

halx99 commented Mar 11, 2026

/clang-format

@axmol-bot
Copy link
Collaborator

👋 @halx99 Command /clang-format received. Running code formatting, please wait...

@axmol-bot
Copy link
Collaborator

✅ Formatting completed and changes have been committed to the PR branch: DrawNode_properties_full_support_remove_clamps.
@halx99 please refresh to see the latest code.

Copy link
Contributor Author

@aismann aismann left a comment

Choose a reason for hiding this comment

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

Approved. Thanks for helping hands

@halx99 halx99 force-pushed the DrawNode_properties_full_support_remove_clamps branch from 640a443 to 3d98444 Compare March 11, 2026 15:42
@halx99
Copy link
Collaborator

halx99 commented Mar 11, 2026

I also fix the duplicated condition check _preserveDrawOrder in drawPoint, please review

image

@halx99
Copy link
Collaborator

halx99 commented Mar 11, 2026

/clang-format

@axmol-bot
Copy link
Collaborator

👋 @halx99 Command /clang-format received. Running code formatting, please wait...

@axmol-bot
Copy link
Collaborator

✅ Formatting completed and changes have been committed to the PR branch: DrawNode_properties_full_support_remove_clamps.
@halx99 please refresh to see the latest code.

@aismann
Copy link
Contributor Author

aismann commented Mar 11, 2026

I also fix the duplicated condition check _preserveDrawOrder in drawPoint, please review

It looks good on my computer. As always, thank you very much.
I will improve some of the DrawNode tests in a new PR in a few days.

@aismann aismann changed the title DrawNode: removed all hardcoded clamps; DrawNode::properties full supported now; DrawNodeTest use ImGUI; DrawNode local properties full supported now; DrawNodeTest use ImGUI; DrawNode: removed all hardcoded clamps; Mar 11, 2026
@aismann aismann changed the title DrawNode local properties full supported now; DrawNodeTest use ImGUI; DrawNode: removed all hardcoded clamps; DrawNode local properties full supported now, removed all hardcoded clamps; DrawNode cpp-tests switched to ImGUI ; Mar 11, 2026
@halx99 halx99 merged commit 58ba339 into axmolengine:dev Mar 12, 2026
27 checks passed
@halx99
Copy link
Collaborator

halx99 commented Mar 13, 2026

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the DrawNode class by replacing the properties struct with direct member variables for drawing parameters such as thickness scale, local scale, pivot, rotation, position, and draw order. The applyTransform method is renamed to applyLocalTransform, and redundant thickness and radius checks are removed from various drawing functions. The TRANSPARENT macro is replaced with Color(). Test cases in DrawNodeTest are updated to reflect these API changes and integrate with an ImGui-based UI. Review comments identify a memory leak in _drawColoredTriangle due to unmanaged dynamic allocation, an inconsistency in the polygon outline width calculation in _drawPolygon that halves the thickness, and an issue where pointSize is incorrectly used as thickness for polygon outlines in _drawPoints, leading to oversized borders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking changes enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DrawNode: thickness is not always correct drawing e.g drawLine(...,1.9f)

5 participants