Конструктор копирования для древовидной структуры иногда удаляет листья

У меня древовидная структура, и мой конструктор копирования, кажется, иногда сбрасывает некоторые из моих «листьев».

Базовая структура:

public class Arrow {
   ArrayList<Arrow> subArrows;
   Interval start;
   Interval end;
}

Конструктор копирования:

public Arrow(Arrow other) {
    this.start = new Interval(other.start);
    this.end = new Interval(other.end);
    if (other.subArrows != null) {
        this.subArrows = new ArrayList<Arrow>();
        for (Arrow sub : other.subArrows) {
            this.subArrows.add(new Arrow(sub));
        }
    } else {
        this.subArrows = new ArrayList<Arrow>();
    }
}

Я ожидал, что это, по сути, сделает глубокую копию моей древовидной структуры. Вместо этого я иногда обнаруживаю, что один из моих массивов subArrows пуст. Я не заметил закономерности, кроме того, что они, как правило, находятся на самом нижнем «уровне» моего дерева.

Любые идеи? Я не использовал java некоторое время.

РЕДАКТИРОВАТЬ: несколько человек просили больше кода, так что вот все места, которые касаются подстрелок. Это из довольно большой структуры алгоритма/данных, поэтому было бы неразумно публиковать все это.

Рекурсивно получает все subArrows и возвращает их набор.

Set<Arrow> allSubArrows(Arrow arrow) {
    Set<Arrow> arrowSet = new HashSet<Arrow>();
    if (arrow.subArrows != null && arrow.subArrows.size() > 0) {
        for (Arrow sub : arrow.subArrows) {
            arrowSet.addAll(allSubArrows(sub));
        }
        return arrowSet;
    } else {
        arrowSet.add(arrow);
        return arrowSet;
    }
}

Мэти объясняет, что это делает, но subArrows изменены внизу:

void enforceMonotonicity(Arrow arrow) {
    boolean changed = false;
    if (arrow.end != null && arrow.start != null) {
        if (arrow.start.isParallelTo(arrow.end)) {
            //either left to right or bottom to top
            if (arrow.start.isVertical()) {
                //left interval pointing to right interval
                if (arrow.start.startGraph.y > arrow.end.startGraph.y) {
                    arrow.end.startGraph.y = arrow.start.startGraph.y;
                    changed = true;
                }
                if (arrow.end.endGraph.y < arrow.start.endGraph.y) {
                    arrow.start.endGraph.y = arrow.end.endGraph.y;
                    changed = true;
                }
            } else {
                //bottom interval pointing to top interval
                if (arrow.start.startGraph.x > arrow.end.startGraph.x) {
                    arrow.end.startGraph.x = arrow.start.startGraph.x;
                    changed = true;
                }
                if (arrow.end.endGraph.x < arrow.start.endGraph.x) {
                    arrow.start.endGraph.x = arrow.end.endGraph.x;
                    changed = true;
                }
            }
        }
    }
    if (changed) {
        //check to make sure SOMETHING is still reachable, if not arrow = null
        if (arrow.start.isVertical()) {
            if (arrow.start.startGraph.y >= arrow.start.endGraph.y ||
                    arrow.end.startGraph.y >= arrow.end.endGraph.y) {
                arrow = null;
            }
        } else {
            if (arrow.start.startGraph.x >= arrow.start.endGraph.x ||
                    arrow.end.startGraph.x >= arrow.end.endGraph.x) {
                arrow = null;
            }
        }
        //if we changed the outer arrows, we need to recursively change the subarrows
        if (arrow != null && arrow.subArrows != null && arrow.subArrows.size() > 0) {
            for (Arrow sub : arrow.subArrows) {
                enforceMonotonicity(sub);
            }
        }
    }
}

Часть алгоритма слияния:

HashSet<Arrow> mergeCells(Set<Arrow> first, Set<Arrow> second) {
    HashSet<Arrow> mergedCell = new HashSet<Arrow>();
    //loop through arrows in adjacent cells and find the ones that connect
    for (Arrow topArrow : first) {
        for (Arrow bottomArrow : second) {
            if(topArrow.start.intersects(bottomArrow.end)) {
                //merge arrows
                Interval middle = topArrow.start.intersection(bottomArrow.end);
                Arrow newArrow = new Arrow();
                if (middle != null) {
                    //if they connect, we copy the two arrows, modify their connection,
                    //create a new arrow with the constituents as subarrows, and add that to the mergedcell
                    //after the mergedcell is created, we can delete the base arrows
                    Arrow topCopy = new Arrow(topArrow);
                    topCopy.start = middle;
                    Arrow bottomCopy = new Arrow(bottomArrow);
                    bottomCopy.end = middle;

                    newArrow.subArrows.add(topCopy);
                    newArrow.subArrows.add(bottomCopy);
                    newArrow.start = bottomCopy.start;
                    newArrow.end = topCopy.end;

                    //if end and middle are parallel, we need to project monotonicity
                    //monotonicity is already enforced within a single cell
                    //and enforcemonotonicity knows whether or not start and end are parallel
                    enforceMonotonicity(newArrow);
                }
                //enforceMonotonicity could null out the new arrow
                if (newArrow != null && !newArrow.isNull()) {
                    mergedCell.add(newArrow);
                }
            }
        }
    }

    //keep the originals in case they can connect later on
    //(hashset doesn't allow duplicates, so no need to worry here)
    mergedCell.addAll(first);
    mergedCell.addAll(second);

    return mergedCell;
}

person Evan Cordell    schedule 11.01.2013    source источник
comment
Похоже, требуется некоторая отладка;)   -  person Oliver Charlesworth    schedule 11.01.2013
comment
Это должно быть хорошо, если вы не испытываете проблем с потоками. Кстати - this.subArrows = new ArrayList<Arrow>(); происходит в обоих случаях - он может появиться за пределами if.   -  person OldCurmudgeon    schedule 11.01.2013
comment
Почему бы вам не создать экземпляр subArrows только один раз перед if?   -  person Diego Basch    schedule 11.01.2013
comment
Просто пропустил это, спасибо. Но на самом деле это ничего не меняет (вероятно, об этом позаботится компилятор?)   -  person Evan Cordell    schedule 11.01.2013
comment
Может ли new Arrow(sub) тихо выйти из строя? т.е. возможно, он выдает исключение, которое перехватывается и отбрасывается в другом месте. В этом отношении - new Interval также может иметь эту проблему (что объясняет, почему массив пуст).   -  person OldCurmudgeon    schedule 11.01.2013
comment
@OldCurmudgeon Это хорошая идея, но, честно говоря, я нигде не ловлю исключений. (Это что-то вроде черновика). Я проверил свой конструктор копирования Interval, и действительно, это была неглубокая копия. Но у меня все еще есть проблема с падением листьев, даже после того, как я ее исправил. Будет отредактировано с большим количеством кода.   -  person Evan Cordell    schedule 11.01.2013
comment
Вы уверены, что действительно удаляете записи? Может быть, ваш метод toString не работает? Как вы обнаруживаете потерю?   -  person OldCurmudgeon    schedule 11.01.2013
comment
Я проверяю структуру в отладчике, и он определенно удаляет записи. По сути, я создаю все возможные листы для каждого возможного допустимого дерева, а затем строю дерево из этих листьев. Не может существовать родитель без базовых дочерних элементов, и дочерние элементы легко идентифицировать (интервалы не охватывают > 1 ни по оси x, ни по оси y, но я понимаю, что это очень специфично для того, что на самом деле делает этот алгоритм). РЕДАКТИРОВАТЬ: я понимаю, что это может показаться неэффективным, но по сути это мемоизация, есть несколько разных деревьев, которые можно построить из одних и тех же базовых листьев.   -  person Evan Cordell    schedule 11.01.2013


Ответы (1)


Когда (other.subArrows == null), ваш код по-прежнему создает this.subArrows на new ArrayList<Arrow>(). Это не настоящий клон (один null, другой НЕ). Если вы хотите убедиться, что subArrow всегда не является нулевым (что облегчает добавление subArrow без проверки null), вы можете создать его экземпляр в определении поля.

Если это все еще не работает, вам, возможно, придется показать больше кода, поскольку мы не знаем, как вы добавляете вложенные стрелки в стрелку. Кроме того, если ваш код находится в многопоточной среде, и ошибку трудно воспроизвести, это может быть проблема с синхронизацией. Обратите внимание, что ArrayList не является потокобезопасным. Вместо этого попробуйте Vector или Collections.synchronizedList или правильно синхронизируйте критический блок. Опять же, у нас нет вашего связанного кода.

Кстати, объявить List<Arrow> subArrows лучше, чем ArrayList<Arrow> subArrows.

person Hui Zheng    schedule 11.01.2013
comment
Эта проблема, хотя и действительная, не может сбрасывать листья, на что жалуется ОП. - person OldCurmudgeon; 11.01.2013
comment
Я не уверен. Это может зависеть от того, как он добавляет subArrows. Вот почему я рекомендую ему показать больше кода. - person Hui Zheng; 11.01.2013
comment
Изменение нулевых подстрелок на пустой список на самом деле то, что я хочу (я понимаю, что это не точная копия). А я вообще не парюсь. Решает ли java поток вещей без моего явного указания? (опять же, это было давно) - person Evan Cordell; 11.01.2013