Startbox API: "Invalid" shapes may cause total luarules failure

[ArchShaman]

https://pastebin.com/tk7ec79n

boxes like this cause total luarules failure. If you comment out the first box's last coordinate, it works fine.

4:42 PM] Cerin: western alliance box 1 is what crashes scenario 1 [4:43 PM] Cerin: all other boxes work ok [4:43 PM] Stentorian: mmmm so i need to redo that box [4:43 PM] Stentorian: can you do me a favor and try to comment the last coordinate of the box? [4:43 PM] Cerin: alright [4:44 PM] Cerin: works now lol [4:44 PM] Stentorian: it's a box shape then. interesting. [4:44 PM] Cerin: pretty nasty that it can produce crashy results like that, not very robust [4:45 PM] Cerin: and the error message in infolog is lol [4:45 PM] Cerin: no information basically

[ArchShaman]

My guess as to the crash is because the shape is like this

[ArchShaman]

I have confirmed this theory just now.

Here's a sample script to induce the bug using a "strange" box with intersecting lines.

local bug = {}
bug[1] = {boxes = {
        {
            {3431, 1147},
            {3762, 650},
            {4034, 944},
            {3596, 1257},
            {3407, 1093}, -- comment me to not crash the game!
        },
    },
}
return bug

Here's what the boxes look like in game

[GoogleFrog]

I am fine with a total luarules failure provided that there is a reasonably explanatory message. Map markers on the box is question would be good.

[ArchShaman]

Detecting the box failure itself would result in a relatively easy fix:

1. Scan lines that make up the polygon of the box
2. If intercepting lines occur, create a new box out of the "weird box"
3. Total failure avoided!

Elaboration of 2: We can see that line 2-3 intercepts line 4-1. So we split the boxes into two: 1,2 + interception of 2-3/4-1 and 3,4 and interception of 2-3/4-1

This provides us with this:

The message that is gotten with the current crash is this:

[f=-000001] Error: [LuaRules::RunCallInTraceback] error=2 (LUA_ERRRUN) callin=Initialize trace=[Internal Lua error: Call failure] error = 2, LuaRules/gadgets.lua, [string "LuaRules/Utilities/numberfunctions.lua"]:99: attempt to index field '?' (a nil value) stack traceback: [C]: in function 'Include' [string "LuaRules/main.lua"]:26: in main chunk ... [f=-000001] Failed to load: minimap_startbox_new.lua ([string "LuaRules/Utilities/numberfunctions.lua"]:99: attempt to index field '?' (a nil value))

This tells us nothing (as mapdevs) about it being a startbox related issue.

[Licho1]

Easy way to test it might be orientation of triplets. Iterate points and for points A,B,C do cross product AB, BC. Negative/positve value signals clockwise/anticlockwise. In a given polygon you expect all triplets to have same orientation

[sprunk]

Triplet orientation might not be the same. Consider this shape:

A +----+ B
  |   /
  |  +  C
  |   \
E +----+ D

The triplet BCD has a different orientation than the others but the polygon is fine.

@ArchShaman I dislike the idea of trying to salvage the invalid box. Consider a star pentagram: how do you split it? Do you include the pentagon in the middle, the spikes, or both? Now consider a star pentagram with the circle around it. Do you include the middle pentagon, the spikes, the outer parts, a combination of the above? I can't think of a case where a mapper deliberately puts an invalid box in the config. Since this is only a problem during map development that can never arise unexpectedly during regular gameplay I think it's ideal to crash so as to make sure good boxes are used, this way there's a guarantee that everything works predictably and robustly during an actual game. I agree with GoogleFrog that ideally there would be markers or generally something obvious to mark the offending box. As far as I can tell, real problem here is that the mapdev tool for drawing boxes does no checks since that's what most people use for designing boxes and aren't getting immediate feedback.

I think there are two tasks here:

• [ ] have the mapdev box drawing widget reject invalid boxes (change color from green to red during drawing, place a marker when finalized, place a marker again when exporting)
• [ ] place markers on the offending box when crashing for invalid boxes that still make it into the config.

[ArchShaman]

Perhaps something like this would be acceptable?

Along with a "Error: Bad configuration detected for box ID blah for Team blah"

Also it would be good if the startbox editor widget also detected this kind of thing with a warning. After reviewing my development stream I did with a friend showing how to do this kind of configuration, I noticed that one of the boxes had a very hard to notice (in the moment) thing like the above. That's the only way I figured out this bug.

@sprunk wouldn't a circle with a star in it just become a circle? Probably better to not try to salvage it, yeah.

[ArchShaman]

this box config also causes a crash for some reason. Have not determined the cause yet.

                [2] = {
                    {3270, 14},
                    {3296, 1917},
                    {5558, 2002},
                    {5806, 1941},
                    {6056, 1955},
                    {6232, 2100},
                    {7032, 2100},
                    {7820, 2203},
                    {7334, 2},
                    {3270, 14},
                },

[sprunk]

This looks good: https://user-images.githubusercontent.com/7841201/50322892-0bb73780-048c-11e9-85a0-6ce97715310c.png

This has a degenerate length 0 edge, the first and the last vertex are the same, remove one of them:

[2] = {
                    {3270, 14},
                    {3296, 1917},
                    {5558, 2002},
                    {5806, 1941},
                    {6056, 1955},
                    {6232, 2100},
                    {7032, 2100},
                    {7820, 2203},
                    {7334, 2},
                    {3270, 14},
                },

[ArchShaman]

I redid the box, and it works now. Thought I'd add another case to the total crashes deal. This one seems very easy to detect. Perhaps "degenerate length 0 edges" could be salvaged?

Something like:

if boxes[#boxes][1] == boxes[0][1] and boxes[#boxes][2] == boxes[0][2] then 
  boxes[#boxes] = nil 
  Spring.Echo("Startbox API Warning: Degenerate length 0 edge detected for box " .. id .. " (AllyTeam " .. allyteamid .. ")")
end

[sprunk]

Yes, this one sounds fine to salvage. This is a bit more general, any N consecutive identical points (not necessarily just 1st and the last) should be treated as one (including in the mapdev widget).

[GoogleFrog]

Salvaging edge cases leads to more blind copypasta as the mappers copy each others boxes without knowing the underlying rules.