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

Crashes only in iPads/live afther - Fix/enhancement for issue #1319 (draw filled complex polygons (concave/convex) #1546

Closed
rarepixels opened this issue Dec 26, 2023 · 15 comments · Fixed by #1549
Labels
bug Something isn't working cocos2dx The issue present in original cocos2dx

Comments

@rarepixels
Copy link

  • axmol version:
    Still using last 1.x, not yet migrated to 2.x,no dev, but wanna give a heads up, in case somebody else have this problem.

  • devices test on:
    Only crashes in iPads / no dev crashes for me, only live crashlytics reports
    No problem at all in android devices or any iPhone model/version

  • developing environments

    • Xcode version: 14.2

I had recently updated my app version with the fix for issuea #1433 and got this crash trigger by poly2tri shapes.h
// Repeat points
"Edge::Edge: p1 == p2"

Removing the changes for concave/convex detection crashes went away.

All crashes are related to the drawPolygon, all in pages with ScrollView,
that calls drawPolygon twice via
Layout::setStencilClippingSize _clippingStencil->drawPolygon

ScrollView:create -> triggers a drawPolygon with 0 filled
scrollView::setContentSize -> in my test case, triggers drawPolygon with screenSize ~ [0,0] [1386,0] [1386,518] [0,518]

Or in the few places where I draw polygons ( only rectangles or round rectangles, so all are convex )

Of course, I can't reproduce the crash and never run into it during development or testing.

From crashlytics:

Device
100% iPad

In my case,
iPad (5th to 10th generation )
iPad Pro ( 2th to 6th )
iPad Air ( 3rd to 4th )
iPad mini ( 4th to 5th )

OS
60% iPadOS 17
32% iPAdOS 16
7% iPAdOS 15
1% iPadOS 14

Version 4.2 has the update #1433 , prev versions, and 4.2.1 do not have that code change.

crash_log_per_version

@halx99
Copy link
Collaborator

halx99 commented Dec 26, 2023

@aismann help to take a look?

@aismann
Copy link
Contributor

aismann commented Dec 26, 2023

Sure. Later.. Im currently on real life

@aismann
Copy link
Contributor

aismann commented Dec 26, 2023

Ok. Here a workaround first:
Add this return true; on DrawNode.cpp line 67 (see below)
It should work again (but the 'concave polygons' be wrong as before)

/** Is a polygon convex?
 * @param verts A pointer to point coordinates.
 * @param count The number of verts measured in points.
 */
static inline bool isConvex(const Vec2* verts, int count)
{
    return true;  // Workaroud for issue #1546

image

@aismann
Copy link
Contributor

aismann commented Dec 26, 2023

The fix in the next PR:
DrawNode 2.0 need a flag setting some methods to the behaviour bevor DrawNode 2.0.

@rarepixels
What's the different between this kind of iPads to the working devices?
Resolution?
Another axmol source path?
Sorry I have not so much time at the moment. Next DrawNode 2.0 PR will fix it (i hope, I cant test it).

@aismann
Copy link
Contributor

aismann commented Dec 26, 2023

> Layout::setStencilClippingSize _clippingStencil->drawPolygon
> 
> ScrollView:create -> triggers a drawPolygon with 0 filled scrollView::setContentSize -> in my test case, triggers drawPolygon with screenSize ~ [0,0] [1386,0] [1386,518] [0,518]

I have found the places which have to be set back to DrawNode 1.0
At least this one:
image
Maybe another potential source lines which can crash (but I don't want adapt it with the next PR)
image

@aismann
Copy link
Contributor

aismann commented Dec 26, 2023

All crashes are related to the drawPolygon, all in pages with ScrollView, that calls drawPolygon twice via Layout::setStencilClippingSize _clippingStencil->drawPolygon

ScrollView:create -> triggers a drawPolygon with 0 filled scrollView::setContentSize -> in my test case, triggers drawPolygon with screenSize ~ [0,0] [1386,0] [1386,518] [0,518]

Or in the few places where I draw polygons ( only rectangles or round rectangles, so all are convex )

@rarepixels
I need your help: Can you log the inputs of drawPolygon like below?

void DrawNode::drawPolygon(const Vec2* verts,
                           int count,
                           const Color4B& fillColor,
                           float borderWidth,
                           const Color4B& borderColor)
{
    for (size_t i = 0; i < count; i++)
    {
        AXLOG("Count %i x: %f y: %f", i, verts[i].x, verts[i].y);
    }
.....

@rarepixels
Copy link
Author

All crashes are related to the drawPolygon, all in pages with ScrollView, that calls drawPolygon twice via Layout::setStencilClippingSize _clippingStencil->drawPolygon
ScrollView:create -> triggers a drawPolygon with 0 filled scrollView::setContentSize -> in my test case, triggers drawPolygon with screenSize ~ [0,0] [1386,0] [1386,518] [0,518]
Or in the few places where I draw polygons ( only rectangles or round rectangles, so all are convex )

@rarepixels I need your help: Can you log the inputs of drawPolygon like below?

void DrawNode::drawPolygon(const Vec2* verts,
                           int count,
                           const Color4B& fillColor,
                           float borderWidth,
                           const Color4B& borderColor)
{
    for (size_t i = 0; i < count; i++)
    {
        AXLOG("Count %i x: %f y: %f", i, verts[i].x, verts[i].y);
    }
.....

Hi @aismann,

I just remove the version with the concave code to avoid the crashes. Luckily for me, the apple guys did the review very fast.

I never manage to reproduce the problem in development with my ipads, but I will try to force it, and if I manage to trigger the crash during this days test, I will send you the logs.

If not, I will setup a live version, that logs the crash, and jumps to the old drawing code, so we can see what goes wrong, but the user will not be affected.

@aismann
Copy link
Contributor

aismann commented Dec 26, 2023

I trink i have found the bug.
Its an Cocos2d-x bug.
Will make a closer look tomorrow.

@rarepixels
Copy link
Author

@rarepixels What's the different between this kind of iPads to the working devices? Resolution? Another axmol source path? Sorry I have not so much time at the moment. Next DrawNode 2.0 PR will fix it (i hope, I cant test it).

A part of the resolution of the device, I have no differences at all in my code, not for android or ios devices,
and I saw no problems on ipad tablets.
No different folders or any change in the code.

For me, there is no hurry at all @aismann, thanks.
I have no complex or concave polygons to draw, so once I commented the code, all crashes were gone.

If you have already find the problem that will be great,
if not I will setup the logs.
Lets see tomorrow :o)

@aismann
Copy link
Contributor

aismann commented Dec 27, 2023

@halx99 please add tags: 'bug', 'cocos2dx '

Fix issue #1546
Wrong use of DrawNode::drawPolygon changed to DrawNode::drawSolidRect

@halx99 halx99 added bug Something isn't working cocos2dx The issue present in original cocos2dx labels Dec 27, 2023
@halx99 halx99 linked a pull request Dec 27, 2023 that will close this issue
4 tasks
@aismann
Copy link
Contributor

aismann commented Dec 27, 2023

@rarepixels
I can't check this fix and I'm not really sure it will catch the error.
But: The coming DrawNode 2.0 PR will have some more checks and I hope you can review the coming PR before it will merged.

@aismann
Copy link
Contributor

aismann commented Dec 30, 2023

@rarepixels ,
Can you test it after merge?
This should FIX your iPad issue.

Can you also log the vertices value?
I think there is another issue with iPads.

halx99 pushed a commit that referenced this issue Dec 31, 2023
* Fix issue #1546 of UILayout(#1549) and small performance boost

* add test

* Committing genbindings changes

---------

Co-authored-by: aismann <aismann@users.noreply.github.com>
halx99 pushed a commit that referenced this issue Jan 3, 2024
* Fix issue #1546 of UILayout(#1549) and small performance boost

* add test

* Update DrawPrimitivesTest.cpp (Smaller tester code)
@rarepixels
Copy link
Author

@aismann , Thanks for your job,
with this holidays I was out for some days.

I will be updating all changes and I will let you know and record the logs, if I found again the problem.

Thanks!

@aismann
Copy link
Contributor

aismann commented Jan 10, 2024

@aismann , Thanks for your job, with this holidays I was out for some days.

I will be updating all changes and I will let you know and record the logs, if I found again the problem.

Thanks!

I hope you never found this issue again ;)
My question is more this:
Why iPads make this wrong values?

@rarepixels
Copy link
Author

No idea, but I will remain also very curious about that....

In my crash reports I found no reason to group the problem, except "device type iPad"
There were crashes in all diferent ipads sizes, models, ios versions, and also CPUs. ( At first I kind of hope that can be the reason )
I quite confident that this has to be an apple problem.
If not, I cant imagine why that code will not create problem in some android devices, or even just once. 👍

The fact that is random, is also weird, as almost no ipad users had the crash repeated.

halx99 pushed a commit that referenced this issue May 22, 2024
* Fix issue #1546 of UILayout(#1549) and small performance boost

* add test

* Update DrawPrimitivesTest.cpp (Smaller tester code)

* Update DrawNodeEx.h

AX_EX_DLL

* Update AXLinkHelpers.cmake

* Update Console.cpp (removed the '\n' which creates an second (empty) line)

* segment stuff

* .

* add some more tests

* Issue  1888

* .

* add all DrawNode cocos2dx

* .

* .

* .

* add feature for drawing in correct order

* .

* Update DrawNodeEx.cpp

* .

* Update DrawNodeEx.cpp

* DRAWNODE_TRIANGLE_ONLY

* DRAWNODE_DRAW_LINE_POINT

* .

* Update DrawNodeExTest.cpp

* Update DrawNodeExTest.cpp

* .

* .

* Add round brackets

* commendet this both lines (check it in next version)

* .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cocos2dx The issue present in original cocos2dx
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants