Introduction
Moderators: dorpond, trevor, Azhrei
Re: Introduction
I don't see the problem with a player moving an NPC token and having that player (and perhaps all players) see the movement metric. Basically the same code that says, "can the player see the token (at all)?" will be used to determine if the distance shows up...
Re: Introduction
One of the issues that came up in the past was when the GM was moving a token near the edge of the fog the movement counter and GM name was showing up in the exposed area even though the token itself wasn't visible to the players.
Obviously the right logic should prevent that from happening but it is something to be careful about.
Obviously the right logic should prevent that from happening but it is something to be careful about.
- jfrazierjr
- Deity
- Posts: 5176
- Joined: Tue Sep 11, 2007 7:31 pm
Re: Introduction
Phergus wrote:One of the issues that came up in the past was when the GM was moving a token near the edge of the fog the movement counter and GM name was showing up in the exposed area even though the token itself wasn't visible to the players.
Yea.. something like that is what I remember, but don't have all the exact details(I knew it had to do with DM and what plaeyrs saw).... which is why I said check with Phergus/Dorpond since I knew they would remember....
I save all my Campaign Files to DropBox. Not only can I access a campaign file from pretty much any OS that will run Maptool(Win,OSX, linux), but each file is versioned, so if something goes crazy wild, I can always roll back to a previous version of the same file.
Get your Dropbox 2GB via my referral link, and as a bonus, I get an extra 250 MB of space. Even if you don't don't use my link, I still enthusiastically recommend Dropbox..
Get your Dropbox 2GB via my referral link, and as a bonus, I get an extra 250 MB of space. Even if you don't don't use my link, I still enthusiastically recommend Dropbox..
Re: Introduction
Here's a patch. 2/3 was synchronizing, although I probably didn't catch it all. 1/3 was a wild guess and I need to delve deeper a bit to understand why this worked. The code I commented out was there presumably for some reason. So test it with some more campaign files.username wrote:I decided to synchronize the access methods. That helped. Unfortunately, that did not fix the root cause.Azhrei wrote:Adding SwingWorkers usually isn't too tough, it's just no one has sat down to do it yet. If you need examples, there are other places in the code that do create SwingWorkers to do off-thread processing.
- Attachments
-
- patch.txt
- (6.09 KiB) Downloaded 110 times
Re: Introduction
Can you explain why you commented it out? That code sets the map zoom level back to what it was in the campaign file after the file is finished loading...username wrote:Here's a patch. 2/3 was synchronizing, although I probably didn't catch it all. 1/3 was a wild guess and I need to delve deeper a bit to understand why this worked. The code I commented out was there presumably for some reason. So test it with some more campaign files.
Regarding the rest of the patch: it looks like you didn't import the Eclipse preferences. If you check the "developers notes" thread (somewhere here!) you'll find there's a set of guidelines for code contributions. Those preferences can be imported by using the file that's included under the maptool/conf directory in the project; the file is eclipse.prefs.import-me.
Looking at the content of the patch, I'm not sure I understand the problem behind loadCampaign (your comments at line 122 in Zone.java). While it's true that loadCampaign() runs in a separate thread, the GUI (i.e. the Graphical User Interface) is blocked already because the glassPane is instantiated. That's the layer that contains the "Loading campaign..." string and the semi-transparent black overlay. Because the glassPane exists and absorbs all mouse and keyboard events, there is no way to interact with the UI. It is true, however, that the window could be resized and that could make additional tokens visible that weren't visible before, but I don't believe repaint events are recognized while the glassPane is active (I believe the first few lines of renderZone() check for the loading state and return prior to drawing anything; yes, they do: see ZoneRenderer.java:902).
If that's the case, how can the loadCampaign() cause a problem?
You said in a previous posting, "While adding some debug prints I found a concurrency violation on Zone.tokenOrderedList in here." Perhaps your print statements were in the wrong place (such as prior to the IF statement in renderZone()?).
I don't believe these concurrency patches are relevant, but I'd be happy to be proven wrong.
Re: Introduction
It solved the problem Apparently something else in the code sets it before the loading thread does. But since I don't know what or whether this alway happens, I have asked for tests. I don't have many campaign files at hand. All that is needed is load them and check that everything is alright.Azhrei wrote:Can you explain why you commented it out? That code sets the map zoom level back to what it was in the campaign file after the file is finished loading...
You are right, I didn't import them. I now did, but nothing happened. (I hope you are not complaining about the unused ZoneRenderer; this is not the final patch.) On the other hand I didn't find the thread you mentioned. I did a global search and stepped through the "developer notes" forum to no avail. Maybe you can provide more detailed directions?Azhrei wrote:Regarding the rest of the patch: it looks like you didn't import the Eclipse preferences. If you check the "developers notes" thread (somewhere here!) you'll find there's a set of guidelines for code contributions. Those preferences can be imported by using the file that's included under the maptool/conf directory in the project; the file is eclipse.prefs.import-me.
The comment was wrong. SwingWorker is to be replaced with EDT. No, the GUI is not blocked. You are just greying everything with it. There are several issues here, which as I said have not completely understood.Azhrei wrote:Looking at the content of the patch, I'm not sure I understand the problem behind loadCampaign (your comments at line 122 in Zone.java). While it's true that loadCampaign() runs in a separate thread, the GUI (i.e. the Graphical User Interface) is blocked already because the glassPane is instantiated. That's the layer that contains the "Loading campaign..." string and the semi-transparent black overlay. Because the glassPane exists and absorbs all mouse and keyboard events, there is no way to interact with the UI. It is true, however, that the window could be resized and that could make additional tokens visible that weren't visible before, but I don't believe repaint events are recognized while the glassPane is active (I believe the first few lines of renderZone() check for the loading state and return prior to drawing anything; yes, they do: see ZoneRenderer.java:902). If that's the case, how can the loadCampaign() cause a problem?
(a) The map panel is being checked within the repaints (isLoading) and is independent of the actual loading thread. A separate thread is started each time, requesting some asset ids. I wonder whether the loading thread doesn't have stuff to do, after the asset ids are all available.
(b) Other panels are still visible and that's where I have seen repaint events are coming in, while loading is not completely done. There's macro processing and what not going on in the background that has effect on the tokens. That is why I needed to synchronize access to the orderedTokenList. The visible area is probably a corollary to that. I don't really know. But someone must have been suspicious, because of the trace string "ZoneView: visible == null. Please report this on our forum @ forum.rptools.net. Thank you!" (BTW, I didn't do any resizing.) Next, lot's of PropertyChangeListeners fire and I can't really follow how they cascade down into the system and obliterate the EDT.
No, they were not in the wrong place. They can never be. All they do is change the timing of the threads and from a programming point this is opaque. You should never rely on assumed behaviour of the scheduler. I hope the explanation above is sufficient to justify the synchronized additions.Azhrei wrote:You said in a previous posting, "While adding some debug prints I found a concurrency violation on Zone.tokenOrderedList in here." Perhaps your print statements were in the wrong place (such as prior to the IF statement in renderZone()?). I don't believe these concurrency patches are relevant, but I'd be happy to be proven wrong.
My current problem is that I don't know who set the correct scale before the loading thread did and will "he" always do so, or is this yet another racing condition. Gotta run though, I'll check that later.
Re: Introduction
tokenLocationCache in ZoneRenderer contains junk data at the first paintComponent for this campaign-file. Anybody have a clue about the intended lifecycle of that member? I still think there's a race condition at the heart of the matter. My current patch clears it at the beginning of renderTokens and that works.
I also found out that the setting of the ZoneScale, the removal of which was part of my first patch, is actually already done one or two lines before the commented line. So it can stay commented out. That makes the affair even more mysterious. There might be a validity toggling of the cache.
I also found out that the setting of the ZoneScale, the removal of which was part of my first patch, is actually already done one or two lines before the commented line. So it can stay commented out. That makes the affair even more mysterious. There might be a validity toggling of the cache.
Re: Introduction
Hm. They should've set up things like code formatting defaults and whether to import individual classes or entire packages. And yes, I didn't point to the specific thread 'cuz I was lazy. I'll find it. I think it's called Developer Guidelines and it comes from the days when I was submitting patches to Trevor and I wanted to know what coding style he preferred/wanted.username wrote:You are right, I didn't import them. I now did, but nothing happened.
Ah. Semantics. I said the "user interface" was blocked, and it is. I did not say that the EDT was blocked. To me a user interface is something the user, er, "interfaces" with. And if all events are being ignored then there's no interfacing going on. I was probably being too literal.The comment was wrong. SwingWorker is to be replaced with EDT. No, the GUI is not blocked. You are just greying everything with it. There are several issues here, which as I said have not completely understood.
I will admit that I don't fully understand the image loading process, but I thought there was a single queue and requests were added to that queue, then the queue was emptied by spawning off threads using one of the (newer) Concurrency libraries. It's certainly possible that those threads wake up code elsewhere, but IIRC they pass an ImageObserver that sets a flag that tells the Concurrency module to generate a window exposure event so that the Zone is repainted. But the Concurrency module doesn't do that until the queue is empty. The end result being that all image loading happens and the Zone isn't repainted until the threads are done. If repaint() is being called prior to that, then the synchronization problem is likely right there...(a) The map panel is being checked within the repaints (isLoading) and is independent of the actual loading thread. A separate thread is started each time, requesting some asset ids. I wonder whether the loading thread doesn't have stuff to do, after the asset ids are all available.
Yeah, that sounds like an ImageObserver triggering repaint events. Sigh.(b) Other panels are still visible and that's where I have seen repaint events are coming in, while loading is not completely done.
Yeah, we had bug reports on it so there were two ways to fix it and I took the simple solution of ignoring it. Bad choice.But someone must have been suspicious, because of the trace string "ZoneView: visible == null. Please report this on our forum @ forum.rptools.net. Thank you!"
But given that I don't really grok the ImageManager and related subsystem 100%, that seemed safer. Especially given how late we are in the 1.3 build cycle.
Not really although I seem to believe that it's generated when renderZone() is called and updated until map panning causes different tokens to be within the visible portion of the Zone component.username wrote:tokenLocationCache in ZoneRenderer contains junk data at the first paintComponent for this campaign-file. Anybody have a clue about the intended lifecycle of that member?
Yeah, but wiping the cache at ever repaint is going to kill the performance of the code that checks to see if there are visible tokens nearby (mostly used by MTscript but isTokenVisible() is called from a lot of places and I believe it uses that cache; could be wrong though, as this is from memory).My current patch clears it at the beginning of renderTokens and that works.
What do you mean by "validity toggling"?I also found out that the setting of the ZoneScale, the removal of which was part of my first patch, is actually already done one or two lines before the commented line. So it can stay commented out. That makes the affair even more mysterious. There might be a validity toggling of the cache.
Good work on this so far! I'm glad someone has taken the effort to look into it -- thanks!
Re: Introduction
Where can I see, whether anything happened at all? Because when importing preferences, it was asking for a "*.epf", which the import-me was not. (At least it didn't find it until I changed the extension to look fo "*.*".Azhrei wrote:Hm. They should've set up things like code formatting defaults and whether to import individual classes or entire packages. And yes, I didn't point to the specific thread 'cuz I was lazy. I'll find it. I think it's called Developer Guidelines and it comes from the days when I was submitting patches to Trevor and I wanted to know what coding style he preferred/wanted.username wrote:You are right, I didn't import them. I now did, but nothing happened.
The distinction is important. The EDT does not only react to user interaction but to programmatically issued redraw events as well.Ah. Semantics. I said the "user interface" was blocked, and it is. I did not say that the EDT was blocked. To me a user interface is something the user, er, "interfaces" with. And if all events are being ignored then there's no interfacing going on. I was probably being too literal.The comment was wrong. SwingWorker is to be replaced with EDT. No, the GUI is not blocked. You are just greying everything with it. There are several issues here, which as I said have not completely understood.
I haven't seen any image problems. Concurrency was with the members I mentioned and that came from PropertyChanges, I believe.. I also haven't seen any concurrency library being used. But then I might be foraging in old code.I will admit that I don't fully understand the image loading process, but I thought there was a single queue and requests were added to that queue, then the queue was emptied by spawning off threads using one of the (newer) Concurrency libraries. It's certainly possible that those threads wake up code elsewhere, but IIRC they pass an ImageObserver that sets a flag that tells the Concurrency module to generate a window exposure event so that the Zone is repainted. But the Concurrency module doesn't do that until the queue is empty. The end result being that all image loading happens and the Zone isn't repainted until the threads are done. If repaint() is being called prior to that, then the synchronization problem is likely right there...(a) The map panel is being checked within the repaints (isLoading) and is independent of the actual loading thread. A separate thread is started each time, requesting some asset ids. I wonder whether the loading thread doesn't have stuff to do, after the asset ids are all available.
Well, do you want the bug fixed or not? The synchronized is a sure thing here. I am certain that you will not see that string again, as long as the synchronizing is observed.Yeah, we had bug reports on it so there were two ways to fix it and I took the simple solution of ignoring it. Bad choice.But someone must have been suspicious, because of the trace string "ZoneView: visible == null. Please report this on our forum @ forum.rptools.net. Thank you!"
But given that I don't really grok the ImageManager and related subsystem 100%, that seemed safer. Especially given how late we are in the 1.3 build cycle.
It is also my impression that the cache is supposed to be used only during the rendering process. But during that rendering process no changes should be made to positions and the like. And after rendering you might as well clear the cache. Therefore all other access to the cache should be removed. That will solve the original problem I set out to tackle. But before changing that behaviour I wanted to get some feedback from the people who "had been there".Not really although I seem to believe that it's generated when renderZone() is called and updated until map panning causes different tokens to be within the visible portion of the Zone component.username wrote:tokenLocationCache in ZoneRenderer contains junk data at the first paintComponent for this campaign-file. Anybody have a clue about the intended lifecycle of that member?
I haven't noticed any difference. Do you have any specific performance tests? I noticed a lot of timers in there but don't know how to read them. I also don't know how my HW relates to the weakest HW you intend to release on. I have Ubuntu 10.1 on i686 (and ran maptool out of the box, ie. no memory increase).Yeah, but wiping the cache at ever repaint is going to kill the performance of the code that checks to see if there are visible tokens nearby (mostly used by MTscript but isTokenVisible() is called from a lot of places and I believe it uses that cache; could be wrong though, as this is from memory).My current patch clears it at the beginning of renderTokens and that works.
I have traced the problem into the renderToken routine. Sometimes the locations for tokens (more precisely, objects on the BG layer; but they all go by the name of token in the code as far as I can tell) are available in the cache, sometimes they are not. For "general repaints" the locations are there, for rescaling events they need to be generated. But for some reason the initial "general redraw" has invalid locations. Off screen? I don't really know and I don't know what separates the rescaling repaints from the ordinary, eg. initiated through map shifts.What do you mean by "validity toggling"?I also found out that the setting of the ZoneScale, the removal of which was part of my first patch, is actually already done one or two lines before the commented line. So it can stay commented out. That makes the affair even more mysterious. There might be a validity toggling of the cache.
Whatever the case may be. It is very tedious to find out the deeper roots for this. As you said, 1.3 is in its final phase. I'd rather carve out the cache and use it locally in paintComponents only.
How does 1.4 profit from 1.3 fixes?
Re: Introduction
It should have updated the user preferences. In particular, the ones regarding how code was formatted (indentation style and which column to insert line breaks) and how the tool modifies code (adding import statements for individual classes instead of all in a package, and trimming spaces from the ends of lines when saving).username wrote:Where can I see, whether anything happened at all? Because when importing preferences, it was asking for a "*.epf", which the import-me was not. (At least it didn't find it until I changed the extension to look fo "*.*".
Some of those are not going to be visible (like spaces at the ends of lines) and others are only applied when something changes (like the import statements). You can verify the import by deleting the import that uses an "*" and then saving the file and seeing how Eclipse regenerates the import statements.
Exactly -- it is important. Which is why I said "GUI" and not "EDT". You are right that repaint events can still be delivered while the "GUI" is blocked because the EDT is not blocked. I was not aware that repaint events are still generated while the GUI is blocked. I'll need to figure out why that would be happening and not being deferred until later.The distinction is important. The EDT does not only react to user interaction but to programmatically issued redraw events as well.
Image problems are the visual manifestation of (most) concurrency bugs in MT. (Obviously, since it's primary purpose is the display of graphics.)I haven't seen any image problems. Concurrency was with the members I mentioned and that came from PropertyChanges, I believe.. I also haven't seen any concurrency library being used. But then I might be foraging in old code.
Yeah, the concurrency library stuff is pretty isolated and was fairly recent. I don't recall where it's actually located...
Of course.Well, do you want the bug fixed or not? The synchronized is a sure thing here. I am certain that you will not see that string again, as long as the synchronizing is observed.
Sounds good to me. Obviously it was added for a reason so I'll need to look at the patch to determine if there's a side effect somewhere, but this sounds like the way to go.[...] Therefore all other access to the cache should be removed.
When running MT go to the menus and choose Collect Performance Data. I think it's on the Tools menu, or maybe the View menu. Now you'll be able to measure the performance difference.I haven't noticed any difference. Do you have any specific performance tests? I noticed a lot of timers in there but don't know how to read them.
I'm not too concerned about that. We have some folks running MT on Java 5 on really old PowerMacs, but they're going to be dead when we move to 1.4 anyway. I'd like them to have a usable 1.3 though.I also don't know how my HW relates to the weakest HW you intend to release on. I have Ubuntu 10.1 on i686 (and ran maptool out of the box, ie. no memory increase).
Hm. I vaguely recall the code you're talking about, but I'll need to look at it to be reminded. I believe the background layer is rescaled and saved in a BufferedImage so that later repaints can simply blit the image into the visible component. Changing the zoom level requires the BufferedImage to be regenerated. Things that users can interact with during normal operation are not part of the BufferedImage (since that would make them more different to manipulate). The same applies to the fog of war re: caching the results.I have traced the problem into the renderToken routine. Sometimes the locations for tokens (more precisely, objects on the BG layer; but they all go by the name of token in the code as far as I can tell) are available in the cache, sometimes they are not. For "general repaints" the locations are there, for rescaling events they need to be generated. But for some reason the initial "general redraw" has invalid locations. Off screen? I don't really know and I don't know what separates the rescaling repaints from the ordinary, eg. initiated through map shifts.What do you mean by "validity toggling"?
You've got that right! Every time I go into the code I find out something else I didn't know about.It is very tedious to find out the deeper roots for this.
As above... Go for it.As you said, 1.3 is in its final phase. I'd rather carve out the cache and use it locally in paintComponents only.
Good question. We expect that much of 1.4 will be written from scratch by the time a final build comes out. There are ideas for redoing the VBL, redoing the way campaigns/maps/tokens are saved and loaded, and taking advantage of new technology like JavaFX. Since it will be built on an OSGi base, much of the internal plumbing will be different.How does 1.4 profit from 1.3 fixes?
I doubt anything done to 1.3 at this point will be carried over as anything more than a learning experience.
Re: Introduction
I am using Eclipse SDK 3.5.2 amd when imprting it says it can't import individual preferences. The set is "empty". Anyway, I replaced the *-imports and blanks with tabs. Hope that was it.Azhrei wrote:It should have updated the user preferences. In particular, the ones regarding how code was formatted (indentation style and which column to insert line breaks) and how the tool modifies code (adding import statements for individual classes instead of all in a package, and trimming spaces from the ends of lines when saving).username wrote:Where can I see, whether anything happened at all? Because when importing preferences, it was asking for a "*.epf", which the import-me was not. (At least it didn't find it until I changed the extension to look fo "*.*".
When finding out how this cascades into other classes, I got worried and took the least invasive approach and only cleared the cache in renderToken. I have the feeling that the cache is not koscher and wrote a comment to that effect.Sounds good to me. Obviously it was added for a reason so I'll need to look at the patch to determine if there's a side effect somewhere, but this sounds like the way to go.[...] Therefore all other access to the cache should be removed.
Attached the patch for the synchronized bits and the main one.
- Attachments
-
- patch.txt
- (5.93 KiB) Downloaded 103 times
Re: Introduction
Here's the other one you asked for. It contains the SystemProperties at the end of the About box/page.
- Attachments
-
- patch.txt
- (2.11 KiB) Downloaded 117 times
Re: Introduction
Thanks. I'll take a look tomorrow night.
Re: Introduction
I guess real life took its toll?Azhrei wrote:Thanks. I'll take a look tomorrow night.
Re: Introduction
Yep, sorry. I was going to try to get a build out tonight but now it's not going to happen. I'm shooting for tomorrow night now.