[patch - MapTool] FoW mods + path computation fix

[JamzTheMan]

There's possibly another bug. Several times today I had MT freeze up when moving tokens and it was always when moving them through FoW and the toggle was OFF.

Also, I seem to be missing the menu option that makes maps Player Viewable on/off? Not sure when I lost it, I need to back out a few patches and see.

[JamzTheMan]

Just a bump for Lee in case he missed it, since Az is building this week. I'd love to see this get into the next patch but it shouldn't given the current issues noted above. (The missing Menu option is an easy fix, you just accidentally commented out an extra line. The MT freezing walking through VBL though is something I didn't look into and is sort of serious.)

[Azhrei]

I'd love to see this get into the next patch but it shouldn't given the current issues noted above.

Agreed. I don't seem to have that patch file in my to-be-applied folder so either I've never downloaded it or it's already been applied and committed.

I can't tell right now as my Java configuration has gotten screwed up. I downloaded and installed the J8 preview release and it has messed up Eclipse something fierce. It appears to be a pathing issue (Eclipse is selecting J8 instead of J7) so I hope to have this fixed soon. Worst case I'll install J7 again. (Edit: No luck so far. About to reinstall J7. Hopefully this fixes it.)

[JamzTheMan]

The last time I applied it (sometime around last edit time of OP), it wasn't committed so I applied it myself. I'm pretty sure you haven't checked it in.

[Lee]

Hi,

I'm sorry for the late reply, it's been toxic all week

@Jamz hmm. May I know the specifics of your movements? i.e. scales - distance, size of movement group etc., mix of snapped and unsnapped tokens etc.

As I've mentioned, the approach was experimental, intended for "reasonable" use, and, honestly, can be greedy as it technically utilizes an unfettered thread pool to spawn at will. I think that, at this stage, there are only 2 options left: I put a cap on the thread pool to limit generation, or we revert to the original method and I just make a patch for the "expose-at-waypoints" feature (thanks for the catch on the mistakenly quoted line btw).

As for the way point info for unsnapped tokens, I believe it's always been the case i.e. I didn't touch those parts of the code. I'll need time to fix it, if ever, so that end-points = waypoints regardless of snapped state. I'm kind of short on time, however, and I'm not sure how this impacts building b90.

[JamzTheMan]

Hi,
I'm sorry for the late reply, it's been toxic all week

@Jamz hmm. May I know the specifics of your movements? i.e. scales - distance, size of movement group etc., mix of snapped and unsnapped tokens etc.

NP, I was able to recreate this with a new map, drew a big square VBL, dropped a token, PC, darkvision, has sight, FoW on, Vision Day, grid 50, snapped to grid, your new option off. Moving through the VBL locks MT up. Note it doesn't seem to spike CPU or memory, just freezes.
*Windows 8x64, Java 1.7_017

screenshot

fow_issue.png (237.86 KiB) Viewed 4472 times

As I've mentioned, the approach was experimental, intended for "reasonable" use, and, honestly, can be greedy as it technically utilizes an unfettered thread pool to spawn at will. I think that, at this stage, there are only 2 options left: I put a cap on the thread pool to limit generation, or we revert to the original method and I just make a patch for the "expose-at-waypoints" feature (thanks for the catch on the mistakenly quoted line btw).

True, although it may just be something more simple, just a uncaught loop or something? It's not doing anything to complicated in my test so I'd expect it to eventually return or spike the cpu if it was doing something crazy?

As for the way point info for unsnapped tokens, I believe it's always been the case i.e. I didn't touch those parts of the code. I'll need time to fix it, if ever, so that end-points = waypoints regardless of snapped state. I'm kind of short on time, however, and I'm not sure how this impacts building b90.

Note* There is still one case where with your option set on, and unsnapped to grid, it exposes NO FoW. With your option off, it performs as normal, exposing FoW at the endpoint, so that part is a bug. The exposing at waypoints in unsnapped state is just a request but low on my personal list as I rarely use unsnapped to grid (one of those, if it's a line or two of code and you are there making changes... )

[Azhrei]

[...] honestly, can be greedy as it technically utilizes an unfettered thread pool to spawn at will.

Ouch! I didn't realize that!

At a minimum we should at least specify that we want a smaller stack size (do some profiling and figure out what kind of stack depth is utilized) so that people who run the JVM with -Xss# where the number is 8M or something don't spend 8M for each of these short-term threads! Folks still running on 32-bit Windows will definitely run out of addressing space and get the "Cannot create native thread" error.

I'm going to leave this patch out until some more testing has been completed.

[Lee]

Thanks Jamz. I'll look into it soon.

@Azhrei: Yeah, I feel the same. Perhaps we should do as I suggested above and revert to the original code for handling FoW exposure? Though it's slower and performance drastically degrades over long paths, it's been used over many versions. I'll just submit a patch to add the bypass / expose-at-waypoints features (macro functions included).

On a side note, I did some profiling trying to catch that bug wolph reported a week or so back. With this patch applied, I was unable to see anything noteworthy with regard to threading. My test parameters might be wrong, in any case. Unfortunately, where I'm bunked at right now, all my machines (virtual and otherwise), are 64-bit systems. It might be a while till I get to a 32-bit machine I can use.

Let me know if the option above is viable. Thanks.

[Azhrei]

@Azhrei: Yeah, I feel the same. Perhaps we should do as I suggested above and revert to the original code for handling FoW exposure?

Agreed. For the time being this patch has not been applied or committed to SVN.

I'll just submit a patch to add the bypass / expose-at-waypoints features (macro functions included).

Okay. No promises about there being a b91 at this point (I'm working on b90 right now) but if there is, this smaller patch can be included.

[Lee]

@Jamz, Azhrei: I was able to figure out the root cause of the lock-up. There's an underlying exception that happens in the original code where the addition of the generated visible area for a step to the overall meta-area is attempted; this is partly caused by the fact that no visible area is generated when a token is in the midst of a solid vbl block. Case in point: There is no lock-up when a token steps into a hollow vbl object.

Anyway, this causes the code to not step into the finally clause where the step-down occurs, making the handler wait indefinitely. Moving the step down out of the clause fixes the lockup issue.

I did some profiling on the submitted patch using the stack size parameter given above and found no cause for concern. Again, I'm using 64-bit machines, so take it with a grain of salt. In any case, there will always be extreme user cases that I cannot envision; to mitigate this, I capped the thread pool to work with 15 threads. Would anyone like a field to enter thread pool size? User groups with higher end machines might benefit.

I'm currently trying to figure out the way point problem. The current official source has not applied my previous path patches, making it more difficult on my part. As I suspected, there's something wrong with way point setting in unsnapped movement, and it happens sometime during the token drag sequence.

Note* There is still one case where with your option set on, and unsnapped to grid, it exposes NO FoW. With your option off, it performs as normal, exposing FoW at the endpoint, so that part is a bug. The exposing at waypoints in unsnapped state is just a request but low on my personal list as I rarely use unsnapped to grid (one of those, if it's a line or two of code and you are there making changes... )

Actually, it's not a bug (unsnapped and exposing fow at end point). I utilized the code for exposing visible areas (exposeVisibleArea), putting in a logical branch for the toggle. If the toggle was not set, it just exposes the visible area where the token is currently at; in this case, the end point, though no actual points were evaluated (because there is not "path" to speak of). If you check the original code, you'd see what I mean.

I'll try to get this in as soon as possible to make it into b90.

Edit: The good news. I fixed all issues reported. The bad news, this is entirely dependent on my earlier path fix where I moved around a lot of stuff to get things. I'm afraid this won't make it into the final build. Sorry.
Edit #2: Just to be clear, the patch involves 13 classes built off what's for b90 so far.

[JamzTheMan]

Re Unsnapped to Grid: Got it, just threw me for a loop that it revealed FoW one way and not the other, didn't realize b89 didn't reveal at all with SnapToGrid off!

Can you still share the updated patch? Or at least the fix for the Lock-up? I'd hate to pull the patch out of my personal build.

[Lee]

@Jamz Sure, but if you applied my other path patch, you'll have to revert to b90 originals prior to applying it as I think it won't apply cleanly due to the SVN change. I also had to convert and apply the changes myself to the aforementioned classes. Since I'll be uploading it later, perhaps Azhrei can still evaluate it for b90?

I'll have it ready when I get home.

[jfrazierjr]

IMHO, the number ONE thing we can and should do is to change the code BACK so that per token exposed area is NOT updated UNLESS there is a server running AND the Individual FOW setting is turned on. GM's who are building their campaign and testing things out should NOT HAVE TO pay a performance penalty for a feature they may not use in their games! That setting is on the server dialog for a reason, so let's respect it and make that the determining factor on if token movement adds exposure information on a per token basis.

[Lee]

@Jamz patch uploaded. Apparently, it was just 12 classes
@jfjrazierjr Sounds good, but how far back would the reversion be? This patch won't be needed by then, would it?

[jfrazierjr]

@jfjrazierjr Sounds good, but how far back would the reversion be? This patch won't be needed by then, would it?

Well.. just to be clear, the change would be manual, not a rollback of SVN to a previous point. There are WAY to many changes since then to try to do a re-merge IMHO and the "fix" would not be more than about 20-30 lines of code anyway, so manual is the way to go. As for your patch being needed, yes, I would say that it could still be helpful. My suggestion is not a performance increase, it's more a matter of shifting the performance hit from everyone as it is today to only those who explicitly choose to take that performance hit. I don't have the code handy and won't for a several days at least(just reinstalled my personal OS at home and have not had time to install Eclipse, SVN, and such and have to much work to do around the house... kitchen still needs a countertop and sink that I need to get completed!!!), but basically, in the places today where we add revealed area to a specific token's "cache", we need to put in a check for the server property Individual FOW = true. If so, proceed as normal. If not, follow the default case and add the updated revealed area cache to the "global exposed area"(ie, the one shared by all tokens.)