ClipWinding: MAX_POINTS_ON_WINDING with moteof (q3) and oasis (wolfet) maps

[illwieckz]

Both ioquake3🗗 and daemon defines MAX_POINTS_ON_WINDING to 64:

https://github.com/DaemonEngine/Daemon/blob/be89425dd92889b0f965d6890a804148b7832711/src/common/cm/cm_polylib.h#L43

But while ioquake3 loads “The Edge of Forever” with that setting, dæmon requires this value to be set to 134.

If this value is less or equal to 133, this message is printed on console and the map loading aborts:

Warn: ClipWinding: MAX_POINTS_ON_WINDING 

There is no reason this map can't be loaded with a max set to 64.

On a side note, because the problem aborts the map loading, this problem must be raised as an error instead of a warning.

How to reproduce

Download map here (it's just a dpk repack of the original pk3 for quake3), load it with +devmap moteof.

[illwieckz]

For some unknown reason, ioquake3 has these functions in cm_polylib.c but dæmon does not have them in cm_polylib.cpp:

RemoveColinearPoints
WindingPlane
WindingArea
WindingCenter
WindingOnPlaneSide
ReverseWinding
ClipWindingEpsilon
AddWindingToConvexHull

See:

• ioquake3:cm_polylib.c
• dæmon:cm_polylib.cpp

[illwieckz]

The functions that are in ioquake3 but not in dæmon were removed in f9d69b9 because being unused.

Some are are dead code in ioquake3 too:

RemoveColinearPoints
WindingCenter
WindingOnPlaneSide
ReverseWinding
ClipWindingEpsilon
AddWindingToConvexHull

But ioquake3 still uses those:

WindingPlane
WindingArea

[illwieckz]

I really have no idea about how to debug this…

The ChopWindingInPlace function does exactly the same on both engines.

[illwieckz]

On a side note, in the past it was an error, not a warning, and it was displayed to the user, see screenshots from Unvanquished 0.35 and Unvanquished 0.17:

It's better to tell the user a cryptic error than just failing silently without notice.

The code that must display such error (instead of a warning printed in console) is:

    if ( f->numpoints > MAX_POINTS_ON_WINDING )
    {
        Sys::Drop( "ClipWinding: MAX_POINTS_ON_WINDING" );
    }

in src/common/cm/cm_polylib.cpp.

[illwieckz]

Unvanquished 0.6.0, the oldest release I have, already raises ClipWinding: MAX_POINTS_ON_WINDING on this map while Tremulous does not (neither XreaL prealpha 20080704).

[illwieckz]

Also, ET:XreaL 0.3.0 20111110 did not have the bug, ET:Legacy does not have it. This looks to be very specific to our tree.

[illwieckz]

With TremFusion 0.99r3_R1422 (linux-x86_64 Jul 6 2009)

I get:

********************
ERROR: Hunk_AllocateTempMemory: failed on 3554440
********************
----- Server Shutdown (Server crashed: Hunk_AllocateTempMemory: failed on 33554440) -----

Edit: it had com_hunkmeg set to 128 by default, if I set it to 256 the map loads in TremFusion.

The more and more I believe this is a bug only our tree has.

[illwieckz]

The error is raised by ChopWindingInPlace() from cm_polylib.cpp.

This function is called in multiple places:

In cm_load.cpp: CMod_CreateBrushSideWindings() (1 time)

In cm_trace.cpp: CM_DrawDebugSurface() (1 time)

In cm_plane.cpp: CM_AddFacetBevels() (2 times), CM_ValidateFacet() (1 time)

The issue occurs when ChopWindingInPlace() is called by CMod_CreateBrushSideWindings() in cm_load.cpp, not in other cases.

Note that the debug code was probably not tested as I suppose it's not part of the default route, but we know for sure the issue does not occurs when called by CM_AddFacetBevels() and CM_ValidateFacet().

[illwieckz]

This CMod_CreateBrushSideWindings() function seems to not exist in ioquake3 or etlegacy, or not with that name. This code is there since first commit on our repository.

[illwieckz]

if I allocate the various arrays formerly using MAX_POINTS_ON_WINDING as number of elements by using 4 * MAX_POINTS_ON_WINDING instead, and keep the > MAX_POINTS_ON_WINDING failure test for everything but that CMod_CreateBrushSideWindings() function, the moteof map loads.

So, we know we don't have regression in code inherited from quake3.

We fail on what looks to be original code. Maybe this original code is not faulty but requires a larger array, who knows?

[illwieckz]

I'm not finding this CMod_CreateBrushSideWindings() function, neither in XreaL, neither in ET:XreaL.

[illwieckz]

After having found it in Dusan's OpenWolf Qio fiork, I doubted this was coming from this at first. Especially since Qio did not have this code. So I've looked to XreaL again. There is multiple branches of it.

And I found it:

git log -S 'CMod_CreateBrushSideWindings'

commit 7a41d76a79ef6015d0bee98350764e539adf3fb9
Author: trebor_7 <trebor_7@8d6dca30-e10d-0410-ba00-f2efb4c278cc>
Date:   Thu Dec 18 22:24:20 2008 +0000

    merged tremulous' 1.1.0 math and collision code

Note that this git commit reference probably only exists on my computer since it's probably a git repository converted from some forgotten svn or even cvs repository dumped from some obscure place of the internet years ago.

What's interesting is that this commit claims this is an import from Tremulous 1.1.0, but the various Tremulous builds I tested did not suffer the issue.

[illwieckz]

Both Tremulous 1.1.0 and Tremulous GPP are able to load the map.

[illwieckz]

So, this is revision 2720 in old svn XreaL repository: https://sourceforge.net/p/xreal/svn/2720

Focus on this function: https://sourceforge.net/p/xreal/svn/2720/tree//trunk/xreal/code/qcommon/cm_load.c?diff=519a30865fcbc9773662b0c7:2719

I don't know if the code from that revision works, but newer versions of XreaL seems to not trigger the bug.

[illwieckz]

I also get this issue with Wolfenstein: Enemy Territory Oasis map:

Warn: ClipWinding: MAX_POINTS_ON_WINDING 

But ET:Legacy loads it well.

[illwieckz]

Do someone has something against using an higher arbitrary value as workaround? Like 256. At least we know the 64 limit was set for another code than the one that is currently hitting this limit. Do someone understands this code?

@zturtleman, as a legendary wizard from mythical kingdom of the third id, maybe with some luck you would have an opinion on the matter?

[zturtleman]

I don't know the polylib code other than it's also used in q3map2 and BSPC. They might use the dead code in ioquake3's cm_polylib.c.

If the code is identical to ioquake3 maybe the error is caused by float related compiler flags. Similar to x87 FPU vs SSE2 (default on x86_64) float differences for BSP patch planes (https://github.com/ioquake/ioq3/issues/186).

ioquake3 x86_64 has -ffast-math which may cause non-standard float behavior. It looks like it's enabled in DaemonFlags.cmake. I don't know what causes Daemon to behavior different from ioquake3.

Maybe try to compare generated assembly code for ioquake3 and Daemon's cm_polylib functions.

[illwieckz]

Hi! Thank you for your answer. The issue is in some code that we don't inherit from ioquake3 (see this comment: https://github.com/DaemonEngine/Daemon/issues/169#issuecomment-578312275) it looks like to have been introduced in XreaL but was borrowed from Tremulous (see https://github.com/DaemonEngine/Daemon/issues/169#issuecomment-578315892). I confirm this code does not exist in ioq3 master but exists in Tremulous 1.1.0 (I just greped the source tarball provided in 1.1.0 release). Edit: commit introducing the CMod_CreateBrushSideWindings code in Tremulous: https://github.com/darklegion/tremulous/commit/611eb5ee8a55d5aa473c48f8f9436a01e3f0cdb7

If you don't understand this code, well, too bad for me and sorry to have pinged you then. I'm looking for someone who can help me to either fix the issue, either give me some clue to where the issue can be or give me some help in understanding what the code does, either tell me it's not an issue. Since Tremulous had the code without failing with this 64 limit, it looks to be an issue anyway.

Unfortunately we can't compare this code to ioq3 (Edit: neither ET:Legacy), but we can compare to Tremulous or XreaL.

[illwieckz]

Note: disabling -ffast-math does not help.

[illwieckz]

Worked around in #312.