FogUtil perf improvement

[ebudai]

Path2D is faster than Area, it can be used in calculateVisibility()

[Phergus]

@ebudai Please use one of the templates and be more descriptive.

Pretty sure that Path2D has been explored in the past and was not used for reasons. Perhaps this has changed now that we are on Java 14. @JamzTheMan ? @cwisniew ?

[ebudai]

My tests show a 5% improvement. I only got the idea looking at another area of the code, but if it introduces a bug, certainly close this.

Also, I will resubmit the issue if you like.

[JamzTheMan]

Area is the slowest. I found the same and if we ripped area out everywhere we would see more improvements.

In fact, using the jts lib may even see better performance.

[ebudai]

I'll have a look at incorporating JTS.

[ebudai]

I have to update Hessian IO, but its used by the clientserver project. Fine to update that one also?

[Phergus]

First step is to open a ticket on the clientserver project explaining the reason to update Hessian IO. Once clientserver has been updated, a release can be done for it at which point the MapTool build can be updated to use the new release.

Second step will of course be testing that all aspects of client/server communication are working correctly.

[Azhrei]

I've tried updating the Hessian server-side stuff before. It was a whole can of worms, with one error cascading into other areas. So instead I've been patching the code when it breaks (as PHP has gone from v5.2 to 7.1).

I'd rather get rid of the Hessian library for talking to the web site and use JSON/REST instead. We'd have to keep existing Hessian in place on the server for older clients, but we could switch to JSON and a new URL for newer code.

[ebudai]

I made a fork, and had no issues updating to the latest Hessian.

edit: that I know of - the error I was getting went away. I did not test any client/server comms - is there an easy way for me to check that out?

[Azhrei]

The error was not related to the updating — that's just copying files into place. It was related to running the PHP code.

I suppose you could modify the URL in the MapTool class so it points to your own private web server...?

[ebudai]

It sounds as if updating the website and related comms code should be its own issue, which would block this one.

[cwisniew]

I made a fork, and had no issues updating to the latest Hessian.

edit: that I know of - the error I was getting went away. I did not test any client/server comms - is there an easy way for me to check that out?

@ebudai I guess there is two parts to test

1. A client MapTool instance with a Server instance just to make sure all the usual things work (connect to server, add a token, add a map, add some macros should cover it)
2. A new server can be registered with an RPTools.net alias, and then a second MapTool instance can see and connect to that server via the RPTools.net alias.

If 2 doesn't work we can create a new issue to update the backend for the server registry

[Phergus]

@ebudai still working on this?

[ebudai]

I had to put this down for a while unfortunately, due to other priorities.

I do plan on resuming work on this item, but it probably won't be for a few weeks.

[Phergus]

No problem.

[Phergus]

@ebudai The next release will be moving to Hessian 4.0.63. Moving this to 1.9.0.

[kwvanderlinde]

Until today, I've never had an issue with FoW performance. But I just tried importing a Dungeondraft map for this weekend's session and, well, the experience was not great.

That said, I tried the original suggestion in this issue: using Path2D instead of Area inside calculateVisibility(). But I actually avoided the Area returned by VisibleAreaSegment::getPath() and didn't bother with any other Areas. The performance boost in my case was significant: 2x instead of 5%.

I don't want to step on any work that @ebudai has already done moving towards JTS. But my change is quite contained, there shouldn't be any significant conflicts.

[ebudai]

Feel free to take this, I got sucked into another project and cannot see when I will be able to finish this issue.

[Phergus]

If it was a Dungeondraft cave map they can be quite brutal as there are a lot of cave well segments which translates to a lot of VBL segments.

[Phergus]

Using the 50x50 Dungeondraft generated cave map from #2716, moving a token from one side to the other and then doing Expose Last Path saw a performance improvement in the neighborhood of 2x. ~9 secs down to ~4 seconds.

On a 100x100 DD generated random dungeon doing the same didn't really show a change but that is also the difference between 4000 VBL points and 131,000 VBL points.

[kwvanderlinde]

Yeah it wasn't an auto-generated cave, but was a hand drawn 70x70 cave map. Still lots of VBL generated for what is otherwise relatively simple geometry. Later today I'll get some more timings on a larger generated cave and see where the bottleneck moves to.

[Phergus]

While importing the dd2vtt file is handy for getting all that tedious vbl in place it's usually easier to export the map from DD with the terrain turned off, take it into GIMP and make a VBL mask out of it then use the VBL mask image to auto-generate the VBL and transfer it down to the map. Cuts the VBL points way down.

[Phergus]

Testing with attached map produces an exception when Exposing Last Path.

org.locationtech.jts.geom.TopologyException: found non-noded intersection between LINESTRING ( 1.0736315132831117E9 2975.0, 5567.0 2975.0 ) and LINESTRING ( 5567.0 2975.0, 5579.0 2975.0 ) [ (5567.0, 2975.0, NaN) ]
	at org.locationtech.jts.noding.FastNodingValidator.checkValid(FastNodingValidator.java:140)
	at org.locationtech.jts.geomgraph.EdgeNodingValidator.checkValid(EdgeNodingValidator.java:81)
	at org.locationtech.jts.geomgraph.EdgeNodingValidator.checkValid(EdgeNodingValidator.java:46)
	at org.locationtech.jts.operation.overlay.OverlayOp.computeOverlay(OverlayOp.java:231)
	at org.locationtech.jts.operation.overlay.OverlayOp.getResultGeometry(OverlayOp.java:183)
	at org.locationtech.jts.operation.overlay.OverlayOp.overlayOp(OverlayOp.java:86)
	at org.locationtech.jts.operation.overlay.snap.SnapIfNeededOverlayOp.getResultGeometry(SnapIfNeededOverlayOp.java:75)
	at org.locationtech.jts.operation.overlay.snap.SnapIfNeededOverlayOp.overlayOp(SnapIfNeededOverlayOp.java:37)
	at org.locationtech.jts.geom.Geometry.union(Geometry.java:1388)
	at net.rptools.maptool.client.ui.zone.FogUtil.calculateVisibility(FogUtil.java:118)
	at net.rptools.maptool.client.ui.zone.ZoneView.getVisibleArea(ZoneView.java:365)
	at net.rptools.maptool.client.ui.zone.FogUtil.lambda$exposeLastPath$1(FogUtil.java:388)
	at net.rptools.maptool.client.ui.zone.FogUtil.exposeLastPath(FogUtil.java:400)

The VBL for this map was produced by MT using the Token VBL generation. Error happens with VBL on token and transferred to map.

A second version was tested that used a mask created from map that produced less VBL points and that map works fine.

The UVTT version exported from Dungeondraft did not produce the exception.

Problem map:
80x80 Cave Sparse Token VBL.zip

Other maps have been fine and the performance improvement runs about 4-5x.

[kwvanderlinde]

I was running into analogous issues while toying with terrain VBL yesterday, so thankfully this wasn't hard to pin down.

[Phergus]

Tested. Exception no longer seen. Performance still good.