Uploaded image for project: 'WorldEditor'
  1. WorldEditor
  2. WED-787

WED apt.dat & gateway import deletes node, resuling in crash

    Details

    • Type: Bug
    • Status: In Progress
    • Priority: Fix ASAP - Hair On Fire
    • Resolution: Unresolved
    • Affects Version/s: 1.5, 1.5.1, 1.6.0b1
    • Fix Version/s: 1.6.0b2
    • Component/s: Import/Export
    • Labels:
      None
    • Severity:
      Crash
    • Platform:
      Mac OS, Windows, Linux

      Description

      WED 1.4 / 1.51 / 1.6, when importing a apt.dat with dupliated (zero length segment) airport line nodes deletes the duplicated nodes upon import, see attached.
      If the apt.dat has a line with only two co-located nodes this creates a single node WED_Chain after import, resulting in WED 1.6.0b1 crashes under windows - confirmed to happen at WED-768, suspect also at WED-780.

      These 2-colocated-node lines are kindof undesireable to start with and not readily produced by user action, but validation in all WED versions prior to WD. 1.6.0b1 (tested all WED 1.6 alphas, WED 1.51 and WED 1.4) will fail to catch them, apt.dat and gateway export will write them without any issues or notifications, see attached earth.wed..xml. A late commit in WED 1.6.0b1 finally added working validation for this and they cannot be exported any more, though still saved.

      At least one such scenery made it onto the gateway: KSEA - ironically it was me submitting it with WED 1.5 and its also in all KSEA demo areas shipped since 1050. I suspect it could be many more - once Tyler Young exports the global airports for the first time with WED 1.6.0b1, he will find out the hard way - validation will now refuse the scenery export.

      The problem upon apt.dat AND gateway import is in WED_AptIE.cpp importLinearPath(). This code intends to only remove rightfully co-located apt.dat lines needed to store the hi/low parts of Bezier nodes, but it will accidentically also delete any non-bezier duplicated nodes, like this apt.dat entry:

      120 Linear Feature 3
      111 49.18323571 -123.17252245 1
      115 49.18323571 -123.17252245
      99

      After import the above results in a single-node line, which will make WED 1.6.0 under windows crash at times immediately after apt.dat import complete, in other cases only after importing e.g. a DSF later on. Under linux no crash at all so far, but I suspect we just got lucky for now ...

      Saving after importing the above apt.dat will create a single node WED_Chain in earth.ewd.xml, which upon re-loading back into WED 1.6.0b1 is rightfully detected as corrupted data, the user is notified and its corrected by deleting this line marking completely.

      Ben Supnik Theodore (Ted) Greene I see several possible actions here:

      1) fix WED_APtIE.cpp importLinearPath() to NOT delete duplicated nodes that are NOT part of a bezier node group

      2) instead of 1) add detection of single-line nodes after apt.dat and gateway import, deleting such accidentially created single-node polygons BEFORE the scenery is drawn the first time (where it may crash). That would be similar to what the earth.wed.xml import currently does.

      3) find out where WED 1.6.0b1 crashes when displaying single-node WED_Chains

      I prefer 1). Thoughts ?

      1. earth.wed.xml
        24 kB
        Michael
      2. test.dat
        0.4 kB
        Michael

        Issue Links

          Activity

          Hide
          bsupnik Ben Supnik added a comment -

          Hi Michael,

          Choice 3 (figure out why this crashes) is not optional! We may decide
          the crash is acceptable, or fix it, and we may also take other action,
          but we definitely do not want to have a crashing code path where we
          don't know WHY it crashes and then code around it...it might be a
          serious bug that we don't get a good peek at later.

          I'll take a look at the crash.

          cheers
          Ben

          Show
          bsupnik Ben Supnik added a comment - Hi Michael, Choice 3 (figure out why this crashes) is not optional! We may decide the crash is acceptable, or fix it, and we may also take other action, but we definitely do not want to have a crashing code path where we don't know WHY it crashes and then code around it...it might be a serious bug that we don't get a good peek at later. I'll take a look at the crash. cheers Ben
          Hide
          tgreene Theodore (Ted) Greene added a comment -

          Hi,

          I tried as hard as I could but could not replicate it.

          Show
          tgreene Theodore (Ted) Greene added a comment - Hi, I tried as hard as I could but could not replicate it.
          Hide
          bsupnik Ben Supnik added a comment -

          I couldn't either - anyone have iron-clad repro steps...if not, we
          should collect a mini-dump?

          With that in mind, reading the bug report more carefully and inspecting
          the WED hierarchy, this is clearly a semantically invalid hierarchy, and
          I'd be shocked if there wasn't code that failed when this happens. So
          we do need to pick one of 1 or 2.

          Ted: I think we already have a validation for this - On OS X, a
          validation check flags the problem.

          Show
          bsupnik Ben Supnik added a comment - I couldn't either - anyone have iron-clad repro steps...if not, we should collect a mini-dump? With that in mind, reading the bug report more carefully and inspecting the WED hierarchy, this is clearly a semantically invalid hierarchy, and I'd be shocked if there wasn't code that failed when this happens. So we do need to pick one of 1 or 2. Ted: I think we already have a validation for this - On OS X, a validation check flags the problem.
          Hide
          meikelm Michael added a comment -

          I get it under wine every time, for this here and for

          WED-780 (which started it all), using XP11pb8 demo area:
          new scenery
          import apt.dat
          import the DSF
          click in hierachy window on the top group created by th DSF import
          crash

          Wine debugger does not yield much, likely because its a stripped executable:

          winedbg: Internal crash at 0x7ece614a

          If I change the two lines #5225, #5552 in that apt.dat by a single digit (so that the two points in the respective airport line items are not identical any more), it works fine.

          Show
          meikelm Michael added a comment - I get it under wine every time, for this here and for WED-780 (which started it all), using XP11pb8 demo area: new scenery import apt.dat import the DSF click in hierachy window on the top group created by th DSF import crash Wine debugger does not yield much, likely because its a stripped executable: winedbg: Internal crash at 0x7ece614a If I change the two lines #5225, #5552 in that apt.dat by a single digit (so that the two points in the respective airport line items are not identical any more), it works fine.
          Hide
          meikelm Michael added a comment - - edited

          I modified a single line to implement 1)
          66464ea8ade5a0208787b98647d0d67ce1909e55

          Also updated the attached earth.wed.xml to include a number of split bezier node items to verify the commit does not alter the behavior wrt those split nodes - just export and re-import apt.dat nothing changes except line L1 does not degenerate any more.

          Show
          meikelm Michael added a comment - - edited I modified a single line to implement 1) 66464ea8ade5a0208787b98647d0d67ce1909e55 Also updated the attached earth.wed.xml to include a number of split bezier node items to verify the commit does not alter the behavior wrt those split nodes - just export and re-import apt.dat nothing changes except line L1 does not degenerate any more.
          Hide
          tgreene Theodore (Ted) Greene added a comment -

          I'm confused how 66464ea8ade5a0208787b98647d0d67ce1909e55 made this fixed. All I see in it is adding a printf statement. Are you sure you have the correct git hash?

          Show
          tgreene Theodore (Ted) Greene added a comment - I'm confused how 66464ea8ade5a0208787b98647d0d67ce1909e55 made this fixed. All I see in it is adding a printf statement. Are you sure you have the correct git hash?
          Hide
          meikelm Michael added a comment - - edited

          I screwed up better than I could imagine before.
          Only part of my 1-line code change (!!!) made it into git for the beta 2. Found the real fix in my old snapshots (long live data hoarding and huge hard drives).

          Show
          meikelm Michael added a comment - - edited I screwed up better than I could imagine before. Only part of my 1-line code change (!!!) made it into git for the beta 2. Found the real fix in my old snapshots (long live data hoarding and huge hard drives).
          Hide
          bsupnik Ben Supnik added a comment -

          This might explain why I couldn't understand how the GIT commit fixed the problem?

          Show
          bsupnik Ben Supnik added a comment - This might explain why I couldn't understand how the GIT commit fixed the problem?
          Hide
          meikelm Michael added a comment -

          Yeah,
          just a few kind words on the console windows are sometimes not enough
          This time I'll put it in and also add the testcase stuff Ted suggested elsewhere. Seems like I need that ....

          Show
          meikelm Michael added a comment - Yeah, just a few kind words on the console windows are sometimes not enough This time I'll put it in and also add the testcase stuff Ted suggested elsewhere. Seems like I need that ....

            People

            • Assignee:
              meikelm Michael
              Reporter:
              meikelm Michael
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated: