Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow setCurrentMap() to use the map Display Name. #2775

Closed
FullBleed opened this issue Jun 21, 2021 · 26 comments
Closed

Allow setCurrentMap() to use the map Display Name. #2775

FullBleed opened this issue Jun 21, 2021 · 26 comments
Assignees
Labels
feature Adding functionality that adds value macro changes This issue adds or changes macro functions. Extra work is required (testing, wiki, code editor) tested This issue has been QA tested by someone other than the developer.

Comments

@FullBleed
Copy link

Is your feature request related to a problem? Please describe.
I use a custom Map Selection macro in my framework and the new map Display Name feature means that I can easily show a list of maps using those names for players. The problem is that I can't use that same list to setCurrentMap() after they select a map by way of the display name. It throws a "Can not find map" error.

Describe the solution you'd like
I would like to be able to use a map's Display Name in setCurrentMap().

Describe alternatives you've considered
I can create some workarounds for my particular use-case, but if there isn't an objection or obvious reason to disallow setting the current map with the Display Name I think it should be allowed.

Additional context
In speaking with MikeP (feature author) a bit before putting out this feature request I found that not using Display Names in macros was, in part, part of the original design so that there could be multiple maps with the same display name if desired. It hadn't, until then, occurred to me that we would be able to do that (because we can't have duplicate true map names.) And while I can see some corner cases where you might want to have two maps with the same display name, I think there will be more cases where it would be better to enforce that display names (like map names) be unique so that they can be used more easily in macros. Furthermore, I think having two maps with the same name in the Map Selection window is very odd and will be prone to cause problems.

Tangential Feature Request: We can get the Display Name with a function using getMapDisplayName(). But we can't pull the true name if all we have is the Display Name. A function to do that would seem prudent... but, of course, probably only if Display Names are unique.

@FullBleed FullBleed added the feature Adding functionality that adds value label Jun 21, 2021
@Azhrei
Copy link
Member

Azhrei commented Jun 21, 2021

What if getInfo("campaign") returned a mapping of each true name and its associated display name? It already includes a list of zones and we couldn't change the structure of that without risking breakage, but another entry could be added. Perhaps a list of objects where each object represented on map with the same data as returned by getInfo("map")...?

@michaelperino
Copy link
Contributor

One core part of the issue I wanted input on was the consensus on should display names be enforced as unique? I think the type of solution that FullBleed was looking for is based on the assumption that Display Names aren't unique. He brought up an excellent point that for players, it isn't intuitive to have a bunch of maps in the lzone selection popup have the same name. At the same time, I could see the DM swapping a map out for a different one while wanting it to keep the same name (for example, a city before and after destruction, etc.) without memory intensive solutions like keeping battlemaps side by side in the same zone.

@Phergus
Copy link
Contributor

Phergus commented Jun 22, 2021

A couple more reasons why someone might want multiple maps with the same display name:

  • Cursed house where as each person goes through a door they all end up alone in their own version.
  • A Mike mentioned, temporal shifts with one or more players end up in earlier or later versions of the map.
  • Like temporal shifts, map versions in alternate realities/parallel universes.
  • Vision quest scenarios.

Though to do those well you really want to have map visibility/access on a per player/token basis.

@FullBleed
Copy link
Author

A couple more reasons why someone might want multiple maps with the same display name:

* Cursed house where as each person goes through a door they all end up alone in their own version.

* A Mike mentioned, temporal shifts with one or more players end up in earlier or later versions of the map.

* Like temporal shifts, map versions in alternate realities/parallel universes.

* Vision quest scenarios.

Yeah... you're going to have to explain to me how having the same name improves the MT experience in any of the scenarios you outline. I mean, players look at the Map Selection and see three maps with the same name and know something is up. Especially when you're going, "Ok, John you switch to the second New York, and Bill you go to the first, Jessica, you go to the third. No John, I said the second New York! Sheesh, dude, pay attention!" At that point you just should name each something else because seeing more than one Map with the same name is a dead giveaway.

In every one of those scenarios it seems like they would be best handled if you build out multiple places on the same map so they at least think they're in the same place (if that's what's important.) Otherwise we need a bunch of other new features like:

Though to do those well you really want to have map visibility/access on a per player/token basis.

Which is a feature I've had to build and led to this feature request. ;) And not being able to setCurrentMap() to to the Display Name or pull the Display Name from the True Name (which all the other map functions are built for) are both issues I've had to workaround. I'm trying to save others the headache here.

@Phergus
Copy link
Contributor

Phergus commented Jun 22, 2021

Though to do those well you really want to have map visibility/access on a per player/token basis.

The explanation was already there and you quoted it.

@FullBleed
Copy link
Author

FullBleed commented Jun 22, 2021

The explanation was already there and you quoted it.

I quoted something that doesn't exist that seemed tacked onto the end of your post when you probably realized most of what you were proposing really wasn't all that viable. Isn't it a little weird to come up with reasons for keeping something being contingent on features that don't (and may never) exist?

I feel certain that we can come up with a lot of hypothetical reasons to have maps with the same Display Name. Most of us are fairly creative. But none that I've seen (so far) are truly better than existing alternatives. So I'm struggling to see what problem it solves (especially compared to the problems it causes.) And I don't, for example, remember ever seeing anyone actually request to allow MT to have maps with the same name before... which indicates to me that despite some nifty corner cases we all might be able to riff on when challenged, there are probably better alternatives to doing it and probably even better reasons not to.

I won't be the first person to start doing some stuff with the map functions and abilities to quickly run into an issue with not being able to use the Display Name. And I won't be the first person to assume that Display Names (like map names) should be unique. These are cases I certainly hadn't thought about until I got my hands into the proverbial pie and tried using the new functions.

So if being able to set duplicate display names is going to stay then I suggest the setMapDisplayName() wiki entry be updated to include that info so people know the option exists. I don't like the trade offs for it, but if it's going to be a thing it should be noted somewhere prominently since it's a feature.

Likewise, it should probably be noted on copyMap, setCurrentMap, and setMapVisible that the Display Name can not be used. When you're building map actions that specifically involve maps that the players can see, and the names that they see, the Display Name could be the string that's "in-hand" (like if you use getVisibleMapDisplayNames)... and since you can't get the true name with the Display Name it means you have to be putting both the true names and display names in lists, then use matching indexes so you can do certain things to the map (copy, change visibility, or make it current.)

@michaelperino
Copy link
Contributor

michaelperino commented Jun 27, 2021

So here's the option that I've been liking best. A function called getMapByDisplay() could be implemented that functions the exact same way as getAllMapNames except you're filtering the returned list by display name. First required parameter is a display name to filter by, second parameter is optional delimiter.

In my test campaign, the setup looks like this.
image

Selected examples:
image

Thoughts? Would that be usable for the case you were working on?

@FullBleed
Copy link
Author

Works for me.

I'm wondering if the function should just be getMapName() though. Seems more consistent with getMapDisplayName() and getAllMapNames().

@michaelperino
Copy link
Contributor

I am worried getMapName would conflict with getCurrentMapName in the minds of players, and getMapName also doesn't give any hints about what piece of data to put in the arguments. Def open to diff names tho.

@Phergus Phergus added this to To do in MapTool 1.10.0 via automation Jun 29, 2021
@Phergus Phergus added documentation needed Missing, out-of-date or bad documentation macro changes This issue adds or changes macro functions. Extra work is required (testing, wiki, code editor) labels Jun 29, 2021
@Phergus
Copy link
Contributor

Phergus commented Jun 29, 2021

My first thought when seeing getMapByDisplay() was, "Get map by which monitor it is on?" Really it is getMapByDisplayName() but that is a bit unwieldy so I can understand why the reduced version.

To be consistent with getMapDisplayName() it could be getMapGmName(). Both indicate what they return versus what the accept as parameters. I think most users will expect the function name to indicate what it is returning when it starts with get.

getMapName() also works and while a bit ambiguous I think most folks would work it out quick enough

@Irarara
Copy link
Contributor

Irarara commented Jun 29, 2021

Another option might be to work that directly into getAllMapNames(). Just add the display name as a second param to filter it. Only issue is that might seem odd since then it's not returning "all" map names (though, it's "all" that meet the criteria...?). Too bad it's not just called getMapNames(). 🤷‍♀️

@Phergus
Copy link
Contributor

Phergus commented Jun 29, 2021

Don't think we'll avoid odd so a pretty good option. At least the name indicates that you can get more than one name in the return.

@FullBleed
Copy link
Author

Another option might be to work that directly into getAllMapNames(). Just add the display name as a second param to filter it. Only issue is that might seem odd since then it's not returning "all" map names (though, it's "all" that meet the criteria...?). Too bad it's not just called getMapNames(). 🤷‍♀️

So what would that look like? getAllMapNames("", displayName) to return a single name?

I think there is a bit of over-thinking going on here.

In the Map Creation window there are two name fields for a Map:

"Name"
"Display Name"

We get the Display Name with: getMapDisplayName(mapName)

So it makes perfect sense that we'd get the map Name with: getMapName(displayName)

MT has a lot of strange function names( i.e. findToken returns the token ID instead of something like getTokenID), but I don't think this would be one of them.

@Irarara
Copy link
Contributor

Irarara commented Jun 29, 2021

Those two aren't equivalents though, one returns a single value and the other would return a list. That's something macro function names always indicate (I can't think up any exceptions off the top of my head at least). So at the very least, it should probably be getMapNames().

For the record though, I'm not too concerned about what ends up getting done. I was just throwing that out there since it came to mind reading this, but it's easy to think of reasons you might not want to do it the way I mentioned (I brought one up, even). As Phergus pointed out, there's no real perfect answer here.

@Phergus
Copy link
Contributor

Phergus commented Jun 29, 2021

getMapNames() works for me.

@FullBleed
Copy link
Author

FullBleed commented Jun 29, 2021

Those two aren't equivalents though, one returns a single value and the other would return a list. That's something macro function names always indicate (I can't think up any exceptions off the top of my head at least). So at the very least, it should probably be getMapNames().

I guess I'm not sure what "list" is being returned.

The primary functionality needed here is to submit a single displayName and get its name just like getDisplayName. That's what we can't do right now (mainly because we can't use certain other single name map functions with the Display Name without getting it's Name). I'm not saying that sending a list of display names and getting a corresponding list of their Names wouldn't have its place, but it wasn't my primary concern when bringing this up (and, above, I see that MikeP mentions a list but also only mentioned one displayName so I think there is some confusion of what the scope of the function is.)

If it's going to work differently than getDisplayName then I guess we should also get getDisplayNames, too. Which I don't think we need... but they would then have the same functionality and naming structure.

So maybe we need to pinpoint what we actually need here.

@michaelperino
Copy link
Contributor

Iraras point was that the function as it currently exists in the screenshots returns all the GM Names associated with a display name (which is not necessarily unique). If we are returning multiple values in some cases, it makes sense to roll with "getMapNames()". However, that also makes your case of building a map select screen difficult because that works under the assumption that duplicate display names are legal. We could maybe include an optional flag after the delimiter that errors out the function if there are duplicate display names detected?

@Phergus Phergus moved this from To do to In progress in MapTool 1.10.0 Jun 29, 2021
@rkathey
Copy link
Contributor

rkathey commented Jun 30, 2021

My suggestion is to make the display names unique. I can see player confusion around "which cave map again? The third one?"

michaelperino added a commit to michaelperino/maptool that referenced this issue Jul 2, 2021
…y like "getAllMapNames" except the list only returns one which share the display name specified in Argument 1. Argument 2 is a optional delimiter. RPTools#2775
michaelperino added a commit to michaelperino/maptool that referenced this issue Jul 2, 2021
…y like "getAllMapNames" except the list only returns one which share the display name specified in Argument 1. Argument 2 is a optional delimiter. RPTools#2775

Swapped out the name of the function to getMapNames instead of getMapByDisp
michaelperino added a commit to michaelperino/maptool that referenced this issue Jul 14, 2021
…y like "getAllMapNames" except the list only returns one which share the display name specified in Argument 1. Also enforces that display names will now be unique. RPTools#2775

Swapped out the name of the function to getMapNames instead of getMapByDisp

Swapped to getMapName since display names are now unique.
@michaelperino
Copy link
Contributor

Display names should now be enforced as unique. It currently shows no warnings when trying to use a duplicate display name under Map Preferences Dialog. When using the setDisplayName command, it returns the display name of the map after attempted update. Should be ready for testing.

While we're at it, do y'all think Map Names (GM Names) should be enforced to be unique? I can't think of many reasons to allow those to be duplicates.

@Phergus
Copy link
Contributor

Phergus commented Aug 8, 2021

While we're at it, do y'all think Map Names (GM Names) should be enforced to be unique? I can't think of many reasons to allow those to be duplicates.

I can't think of a good (or even not very good) reason to allow duplicate GM names.

@Phergus
Copy link
Contributor

Phergus commented Aug 8, 2021

When trying to set a duplicate Display Name with setMapDisplayName() there is no error message or indication that the name change has failed. This needs to be changed. At the very least a localized message saying something like, "That is already used on a different map." Bonus points for naming the other map.

@michaelperino
Copy link
Contributor

Fair enough, the current implementation returns the display name of the map but I see how that leaves space to be desired. I'll look at it within the next few days.

michaelperino added a commit to michaelperino/maptool that referenced this issue Aug 15, 2021
…y like "getAllMapNames" except the list only returns one which share the display name specified in Argument 1. Also enforces that display names will now be unique. RPTools#2775

Swapped out the name of the function to getMapNames instead of getMapByDisp

Swapped to getMapName since display names are now unique.

2021-08-15 Error now provided if a display name is selected that is a duplicate. Tooltip updated to alert the player to this behavior within Map Properties Dialog. I'm unsure if it would've updated across the other languages that don't have translations yet so I manually dropped it there as well.
@michaelperino
Copy link
Contributor

Can this get closed btw? I think the issue Phergus brought up was addressed in the August merge but I'm unsure if it was tested.

@Phergus
Copy link
Contributor

Phergus commented Sep 30, 2021

@michaelperino it hasn't been tested yet and will get the tested Label when it happens. Is the wiki page up-to-date for the function?

@Phergus
Copy link
Contributor

Phergus commented Sep 30, 2021

Tested. Attempting to set a duplicate map display name now returns an error message noting the name is in use.

@Phergus Phergus moved this from In progress to Review in progress in MapTool 1.10.0 Sep 30, 2021
@Phergus Phergus added the tested This issue has been QA tested by someone other than the developer. label Sep 30, 2021
@Phergus Phergus moved this from Review in progress to Reviewer approved in MapTool 1.10.0 Sep 30, 2021
@michaelperino
Copy link
Contributor

It reads
"Introduced in version 1.10.0
Returns the GM name of a given map. If the specified map does not exist, an error is produced."
I can go get the actual error when I get home if that's the standard for the wiki.

https://wiki.rptools.info/index.php/getMapName

@Phergus Phergus removed the documentation needed Missing, out-of-date or bad documentation label Oct 2, 2021
@Phergus Phergus closed this as completed Oct 2, 2021
@Phergus Phergus moved this from Reviewer approved to Done in MapTool 1.10.0 Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adding functionality that adds value macro changes This issue adds or changes macro functions. Extra work is required (testing, wiki, code editor) tested This issue has been QA tested by someone other than the developer.
Projects
No open projects
Development

No branches or pull requests

6 participants