Modify

Opened 3 years ago

Closed 3 years ago

#13306 closed enhancement (fixed)

Make map paint code use double coordinates

Reported by: michael2402 Owned by: team
Priority: normal Milestone: 16.08
Component: Core mappaint Version:
Keywords: gsoc-core Cc: Don-vip, bastiK, stoecker

Description

This patch makes the map paint code use double coordinates.

It prevents the MapView from rounding. We use Path2D (GeneralPath) for drawing most of the time so there is no change in the computations needed, we simply skip a rounding step.

I moved some computations to a common place. I moved the code to compute a symbol shape to Symbol.

I use the MapViewPoint class in many places. It is an immutable Point2D that can be used to get the view and the east/north or lat/lon coordinates. That way, we do not need more intermediate objects. The MapPath2D has a convenience method to add such points.

A new ArrowHelper draws the arrows. You simply tell it the point the arrow should point to and the direction it should point from.

Attachments (3)

patch-mapview-use-state-in-renderer.patch (105.2 KB) - added by michael2402 3 years ago.
patch-patch-mapview-use-state-in-renderer-2.patch (104.6 KB) - added by michael2402 3 years ago.
patch-fix-13306.patch (104.6 KB) - added by michael2402 3 years ago.

Download all attachments as: .zip

Change History (20)

Changed 3 years ago by michael2402

Changed 3 years ago by michael2402

comment:1 Changed 3 years ago by Don-vip

the patch does not apply anymore, can you please update it?

Changed 3 years ago by michael2402

Attachment: patch-fix-13306.patch added

comment:2 Changed 3 years ago by michael2402

I updated that one, too.

comment:3 Changed 3 years ago by Don-vip

Resolution: fixed
Status: newclosed

In 10827/josm:

fix #13306 - Make map paint code use double coordinates (patch by michael2402) - gsoc-core

comment:4 Changed 3 years ago by Don-vip

In 10829/josm:

see #13306 - fix wrong version number

comment:5 Changed 3 years ago by Don-vip

Resolution: fixed
Status: closedreopened

comment:6 Changed 3 years ago by Don-vip

Summary: [Patch] Make map paint code use double coordinatesMake map paint code use double coordinates

Do we really want to deprecate the getPoint() methods? It is used 90 times in JOSM, even more in plugins. I looked at the first occurence in JOSM code (ButtonMarker.java), there is no added value to replace

    @Override public boolean containsPoint(Point p) {
        Point screen = Main.map.mapView.getPoint(getEastNorth());
        buttonRectangle.setLocation(screen.x+4, screen.y+2);
        return buttonRectangle.contains(p);
    }

by:

    @Override public boolean containsPoint(Point p) {
        Point2D screen = Main.map.mapView.getPoint2D(getEastNorth());
        buttonRectangle.setLocation((int) screen.getX()+4, (int) screen.getY()+2);
        return buttonRectangle.contains(p);
    }

comment:7 Changed 3 years ago by michael2402

... it depends on if we want to encourage such use. If we simply change the code like you suggested, we won't gain anything.

  1. containsPoint is not able to handle multiple map views.
  2. containsPoint does not respect the border
  3. The click handling on the map view is a bit (or a lot) hacked right now - the best thing would be to register a single listener on the mapView. We could then add a new method to the layer, like handleClick() or handleContextMenu(). This would even allow us to overlay multiple context menus for the imagery layers.
  4. We should compute the map position for every click first. So we have boolean containsPoint(MapViewPoint p)
  5. You can change this method to use a local Rectangle2D - this gives the JIT the ability to inline more and optimize away the object creation
  6. We can add a method MapViewPoint#rectAround(width, height) to quickly create that rectangle.


In plugins, I even found uses of mv.getPoint(Main.getProjection.latlon2eastnorth...)

Other examples that currently use the method:

  • GPX: DrawArrows: Use the new draw helper class. We have several places where such arrows are drawn.
  • GpxDrawHelper: In drawLines, we can use a Path2D
  • GpxDrawHelper: In drawPoints, e.g. g.fill(screen.rectAround(largesize, largesize))
  • DrawAction: It already uses a Path2D. Simply change to Point2D - or simply MapViewPoint, so you do not even need the .getX()

But you are right, there are some situations (e.g. painting the GUI buttons) where integer coordinates are easier to handle and potentially faster.

comment:8 Changed 3 years ago by Don-vip

OK. Well until we remove all uses of these methods in JOSM core, I'm removing the deprecation warning, it causes too much noise. It can be added back at the same time the method is no more used in core.

comment:9 Changed 3 years ago by Don-vip

In 10843/josm:

see #13306 - remove (temporarily ?) deprecation warning for getPoint methods, still massively used in JOSM core

comment:10 Changed 3 years ago by Don-vip

I don't understand what's going on with LineClip. Your patch has modified it, but the class is not used anymore. As a result the optimized build process removes it, and we have #13392.

Is the class still needed?

comment:11 Changed 3 years ago by michael2402

I don't know if any plugin uses it. As far as I can tell, the overflow problem is solved by using double instead of int coordinates, so it is not needed since Java2D does the right clipping on it's own.

comment:12 in reply to:  11 Changed 3 years ago by Don-vip

Replying to michael2402:

it is not needed since Java2D does the right clipping on it's own.

Are we sure of that? I see in the removed code this mention which refers to #4289 and #4424:

                                /**
                                 * Do custom clipping to work around openjdk bug. It leads to
                                 * drawing artefacts when zooming in a lot. (#4289, #4424)
                                 * (Looks like int overflow.)
                                 */
                                LineClip clip = new LineClip(p1, p2, bounds);

Today we have #13385 which seems related, what do you think?

comment:13 Changed 3 years ago by michael2402

#13385 is not related to clipping. LineClip did only clip the part of the line that was out of view, in #13385, all lines are inside the view.

comment:14 Changed 3 years ago by Don-vip

OK let's remove it then.

comment:15 Changed 3 years ago by Don-vip

In 10857/josm:

see #13306 - remove unused LineClip class

comment:16 Changed 3 years ago by Don-vip

In 10858/josm:

see #13306, see #13392 - remove Main.isOpenJdk, not used anymore

comment:17 Changed 3 years ago by Don-vip

Resolution: fixed
Status: reopenedclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.