0

So I have three classes. On the one hand there is the Point class:

public class Point {
    public float x;
    public float y;

    // Every point has a list of connections to other points indicated
    // by their position in the list of points in the main class.
    public List<Integer> connections = new ArrayList<>();

    public Point(float x, float y) {
        this.x = x;
        this.y = y;
    }
}

On the other hand there is the Line class:

public class Line {
    public Point start;
    public Point end;

    public Line(Point start, Point end) {
        this.start = start;
        this.end = end;
    }
}

And the Combination class:

public class Combination {
    public Line a;
    public Line b;

    public Combination(Line a, Line b) {
        this.a = a;
        this.b = b;
    }
}

I want one method to return a list of all possible combinations based on lines which are generated from the given points. My current approach is the following:

public static List<Combination> findCombinations(List<Point> points) {
    List<Combination> combinations = new ArrayList<>();
    List<Line> lines = new ArrayList<>();

    for (Point a : points) {
        for (int p : a.connections) {
            lines.add(new Line(a, points.get(p)));
        }
    }

    for (Line a : lines) {
        for (Line b : lines) {
            if (a == b) continue;
            combinations.add(new Combination(a, b));
        }
    }

    return combinations;
}

I have a base class containing a list of points. For every instance of that class I call the method above once only. However, a lot of instances are created and this method slows down the process.

I need to find the combinations in order to test if there is a intersection for the two lines (segments) in each Combination instance. I basically want to find all combinations of lines that intersect. To achieve this I need to get the combinations and then check where the intersection is.

The calculation of the intersecting point is not the problem.

bw2801
  • 321
  • 2
  • 8
  • 15
  • 1
    I think you rather want to `continue` instead of `break` since this wont produce every combination. – SomeJavaGuy Oct 21 '15 at 12:53
  • 2
    @JonK In this case it's okay to use `==` since you're iterating across the same list twice and you really only want to avoid picking the same element twice. – biziclop Oct 21 '15 at 12:54
  • In other words: You want to create a list of Combinations containing all possible paths traversing a point. Where the path contains the last line beofre the points and the outgoing line. – AlexWien Oct 21 '15 at 12:54
  • @biziclop Good point - I hadn't noticed that both loops were iterating the same collection! – JonK Oct 21 '15 at 12:55
  • I'm wondering though whether you really need to generate and store all possible combinations. After all, you can generate them at the point where you're using them. – biziclop Oct 21 '15 at 12:55
  • @JonK if both `Point` and `Line` don´t override `equals` it would be the same even so. – SomeJavaGuy Oct 21 '15 at 12:56
  • Just a thought - do you actually need a list of all possible combinations, or do you just need to know how many combinations there are in total? – JonK Oct 21 '15 at 12:58
  • 1
    Are you using Java 8? Helps to know if lambdas are a choice. – MadConan Oct 21 '15 at 12:58
  • @JonK I need a list of all combinations – bw2801 Oct 21 '15 at 13:00
  • @MadConan yes I am using Java 8 – bw2801 Oct 21 '15 at 13:01
  • @bw2801 To add onto my first point, you will also create multiple duplicates of one single combination with this implementation. – SomeJavaGuy Oct 21 '15 at 13:01
  • Your `Combination` class allows two lines that have no points in common. Is that intended? – MadConan Oct 21 '15 at 13:03
  • [This question](http://stackoverflow.com/q/58306/3419894) might be worth a read - it's not specifically about Java but it sounds similar to what you need, might be useful. – JonK Oct 21 '15 at 13:03
  • @MadConan yes that is intended. – bw2801 Oct 21 '15 at 13:06
  • For what do you need that for? – AlexWien Oct 21 '15 at 13:12
  • I just edited the question to show the whole method. – bw2801 Oct 21 '15 at 13:13
  • @AlexWien I have a base class containing a list of points. I want to get the combinations in order to calculate intersection points (if they exist). For every instance of this base class the method is called once only. However, a lot of instances will be created and this is one of the most time consuming methods in my application. – bw2801 Oct 21 '15 at 13:17
  • @MadConan It would NOT be the job of the class garuentee that they have a point in common,. This is usually ensured during build up of that structure,. – AlexWien 5 mins ago – AlexWien Oct 21 '15 at 13:18
  • 1
    Why dont you mentioned this before? Instead of showing your implementation, you should have described ypour task to be solved. Please describe in more detail about to find all intersections of all lines? Do you want to find all intersetcions oinbt of all point to point combination? – AlexWien Oct 21 '15 at 13:20
  • @AlexWien It depends on what the class is meant to represent. If it should represent a pair of connected line segments, the class should check for that invariant (namely that `a.end == b.start`) in the constructor. But if it's just a container for a tuple of line segments, then obviously such checks are not needed. – biziclop Oct 21 '15 at 13:24
  • @AlexWien I edited the question to add more information related to the purpose of finding the combinations. – bw2801 Oct 21 '15 at 13:26
  • You might also want to take a look at the [Bentley-Ottmann algorithm](https://en.wikipedia.org/wiki/Bentley%E2%80%93Ottmann_algorithm), which in general is more efficient than the brute-force algorithm of intersecting every line segment with every other. – biziclop Oct 21 '15 at 13:40
  • @biziclop if i understand it right, this algorithm expects that any two points never have the same x value. I can't guarantee that. – bw2801 Oct 21 '15 at 13:55
  • @bw2801 There are ways of dealing with this situation, described later on. Even if you choose the naive algorithm, you're definitely better off checking intersections without creating a long list of `Combination` objects first. Another "quick win" is not to check every line with every other line, just the ones that come later in the list. In the current setup you check every line pair twice: once as `(a,b)` and then as `(b,a)` – biziclop Oct 21 '15 at 14:07
  • Im am sure you mean "line segment" intersection, not line intersection. line segments are limited to the part between the points, while lines are infinite – AlexWien Oct 21 '15 at 14:30
  • What is the number of expected points? If less than 1000 it snot worth to do an adavnced algorithm (depending how often you need that task) – AlexWien Oct 21 '15 at 14:34

2 Answers2

2

I such graphs, formed by points and lines (often called nodes and edges) the desired combinations are usually iterated on the fly. So usually one should not genereate an explicit list containing all such combinations.

And if you insist in creating all combinations, there is no faster way than iterating through all combinations. (Update: If your task is to find all line intersections there are much faster well known algorithms. This field is called Compuational geometry)

You can speed up a bit if the graph is fixed, so it does not change anymore after creation.
In that case you can replace the

 public List<Integer> connections = new ArrayList<>();

with the faster array of int, which uses a quarter of the memory, too:

public int[] connections.

This optimization usually needs two steps. Build up your graph with ArrayList, then after it is ready, convert all to an static graph using the int[]

Further you are mentioning "efficency". Especially here, one have to decide between memory effciency (eg as used an embedded navigation system) or calculation speed (as used when having much memory).

Your solution, and my answer are related to calculation speed.

AlexWien
  • 28,470
  • 6
  • 53
  • 83
0

If the intent is to combine the newly created line with all other lines, you can get away from the second structure by incorporating the creation of the Combination from the new Line and all previous ones. I'm not sure how much faster this would be (if at all), but I feel it's a bit better in terms of code organization.

public static List<Combination> findCombinations(final List<Line> lines, final List<Point> points) {
    final List<Combination> combinations = new ArrayList<>();

    for (Point a : points) {
        for (int p : a.connections) {
            Line l = new Line(a, points.get(p));
            for(int i=0; i<lines.size(); i++){
                combinations.add(new Combination(lines.get(i),l));
            }
            lines.add(l);
        }
    }

    return combinations;
}
MadConan
  • 3,749
  • 1
  • 16
  • 27
  • Currently the existing lines are not passed as parameter. If I did that I'd have to make another loop outside of this method and this would be even more nested, wouldn't it? – bw2801 Oct 21 '15 at 13:57
  • No. Functionally, it won't matter if you are accessing a class member or a method argument. The behavior should be the same. – MadConan Oct 21 '15 at 13:58
  • There is a slight performance benefit to passing objects as method parameters as opposed to accessing global ones. – MadConan Oct 21 '15 at 13:59