axmolengine / axmol

Axmol Engine – A Multi-platform Engine for Desktop, XBOX (UWP) and Mobile games. (A fork of Cocos2d-x-4.0)
https://axmol.dev
MIT License
868 stars 195 forks source link

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

Closed rarepixels closed 9 months ago

rarepixels commented 9 months ago

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 commented 9 months ago

@aismann help to take a look?

aismann commented 9 months ago

Sure. Later.. Im currently on real life

aismann commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago
> 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 commented 9 months ago

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 drawPolygonlike 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 commented 9 months ago

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 drawPolygonlike 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 commented 9 months ago

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

rarepixels commented 9 months ago

@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 commented 9 months ago

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

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

aismann commented 9 months ago

@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 commented 9 months ago

@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.

rarepixels commented 8 months ago

@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 commented 8 months ago

@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 commented 8 months ago

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.