[Marble-devel] Review Request 123181: Added Right placement for labels and placemark sorting

[Dennis Nienhüser]

> On March 30, 2015, 1:59 p.m., Dennis Nienhüser wrote:
> > src/lib/marble/PlacemarkLayout.cpp, line 43
> > <https://git.reviewboard.kde.org/r/123181/diff/1/?file=359472#file359472line43>
> >
> >     I'd call it `hasRoomFor` for readability and pass both parameters as const references for performance.
> 
> Adam Dabrowski wrote:
>     I agree with the name, but not with passing a QVector as const reference. It is implicitly shared so there is no point. Will change QRectF to const reference, though these kind of optimizations IMO should be only done when there is anything to gain.

I disagree wrt. not passing const references:
- copying QVector does not copy the data directly, true. However the remaining meta data (a couple of ints and uints and an atomic int) are duplicated, so more memory has to be touched compared to a reference
- due to the instance duplication and implicit sharing the atomic int for reference counting is increased once and decreased again at the end of the method. Atomic int operations are not cheap.
In my opinion it is similar to the "prefer prefix increment to postfix increment" rule: It might look slightly odd on first sight, but you get used to it and there's always a benefit. Might be a small benefit in most situations, but will safe your ass the other time -- I remember a situation when I had to refactor my code to pass shared pointers by const reference because atomic int operations involved by passing parameters by-value were taking nearly all CPU time.


- Dennis


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123181/#review78214
-----------------------------------------------------------


On March 31, 2015, 7:20 a.m., Adam Dabrowski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123181/
> -----------------------------------------------------------
> 
> (Updated March 31, 2015, 7:20 a.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> -Added Right position for placemark labels. Up to 7 positions are available for labels if placemarks overlap.
> -Added sorting of placemarks before layouting. Without this there would be labels "jumping" around as several moving placemarks overlap (now they are drawn in the same order and there is no jumping). Cost of sorting is miniscule compared to other operations performed when layouting.
> -Refactored code for checking if layout space is already occupied
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/PlacemarkLayout.cpp 6c0af30 
>   src/lib/marble/geodata/data/GeoDataLabelStyle.h 046f7bc 
>   src/lib/marble/geodata/graphicsitem/GeoLineStringGraphicsItem.cpp 5949c7d 
> 
> Diff: https://git.reviewboard.kde.org/r/123181/diff/
> 
> 
> Testing
> -------
> 
> Displayed 1, 2, 3 and 7+ items.
> Checked for no flickering.
> 
> 
> Thanks,
> 
> Adam Dabrowski
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20150401/f0f5c59c/attachment.html>