Optimize linestring rendering via automatically assigned detail level values.

Review Request #123239 - Created April 3, 2015 and updated

Information
Torsten Rahn
marble
Reviewers
marble
beschow, nienhueser, stanciu, tgridel

Allow for automatically assigning detail values to the nodes inside a linestring.
This mechanism is used for PN2 parsing in this patch.
Taking the detail level into account this approach improves geometry layer performance by 20% on my i7 based machine.
This patch also fixes the "backwards" semantical meaning of ViewportParams::resolve()
In the next step this optimization could also be applied during KML and OSM parsing.

Tested manually using the Atlas map, Plain map and Political map without apparent regressions. "make test" runs fine.
Checked the geometry layer values by starting Marble with --runtimeTrace. On average the msec values are about 20% lower on my machine (and I hope that improvement is even better on slower CPUs).
Ideas for unit tests regarding this patch are welcome. ;-)

Issues

  • 6
  • 1
  • 1
  • 8
Description From Last Updated
curly brackets please Dennis Nienhüser Dennis Nienhüser
Let's have a QVector<double> m_resolution member and place the values there, then replace the implementation with sth like return m_resolution.value(level, ... Dennis Nienhüser Dennis Nienhüser
curly brackets please and move to the top of the method Dennis Nienhüser Dennis Nienhüser
curly brackets please Dennis Nienhüser Dennis Nienhüser
newline missing Dennis Nienhüser Dennis Nienhüser
why not share with geodatalinestringprivate somehow? Dennis Nienhüser Dennis Nienhüser
Torsten Rahn
Torsten Rahn
Review request changed

Description:

   

Allow for automatically assigning detail values to the nodes inside a linestring.

    This mechanism is used for PN2 parsing in this patch.
    Taking the detail level into account this approach improves geometry layer performance by 20% on my i7 based machine.
~   This patch also fixes the "backwards" semtantic meaning of ViewportParams::resolve()
  ~ This patch also fixes the "backwards" semantical meaning of ViewportParams::resolve()
    In the next step this optimization could also be applied during KML and OSM parsing.

Torsten Rahn

Issues with this patch:
- Antarctica isn't rendered correctly
- It might be useful to change the algorithm so that it properly handles cases more decently with 3-4 nodes

Dennis Nienhüser

what's needed to fix the poles? special-case it and don't optimize around there as a dirty hack maybe?

  1. I need to check. Can't be rocket science though. :)

src/lib/marble/ViewportParams.cpp (Diff revision 1)
 
 

did you catch all usage of this function? It's a public method, I wonder if that subtly introduces bugs in 3rd party stuff if we don't rename it at the same time.

  1. Yes it's used only in one place. The semantic is backwards.

  2. Ah, now I see the catch, we have three overloads of resolve(). Go ahead, makes sense.

curly brackets please

This is like a cache with 1 entry? Does using a QCache make sense?

  1. no it's just storing a single previous value for comparison.

What's the reason not to use a function here? Is it supposed to be faster, more readable, or to allow fine-tuning of values?

What about using a QMap<int,double> instead and calling lowerBound() to return the level? That is as readable and fine-tunable and faster wrt. computational complexity (for large input sizes at least which arguably is not the case here)

  1. The reason not to use a function here is that this makes it easier to tweak and fine-tune single values based on trial and error testing on the screen. Using a function this would become harder to understand and would be less transparent.
    Yes, using QMap could be a possible alternative - and in this case it would certainly be a bit slower for most cases - no idea whether it would be more readable.

  2. Ok, fine.

Let's have a QVector<double> m_resolution member and place the values there, then replace the implementation with sth like

return m_resolution.value(level, m_resolution[17]);
  1. That's indeed a better solution - I will change it accordingly.

curly brackets please and move to the top of the method

curly brackets please

newline missing

why not share with geodatalinestringprivate somehow?

  1. I thought about it. But I'd like to keep this stuff still in private classes. Also putting it into GeoDataLineString would require to make this method static. So the current solution seems to be more appropriate.

  2. What about having a comment on top of each pointing to the other? I'd like to avoid them going out of sync.

Dennis Nienhüser

This patch has been pushed, right?

Loading...