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

2.6 Combat Move units with a double click - units are truncated & ... #12447

Closed
TheDog-GH opened this issue Mar 19, 2024 · 25 comments
Closed

2.6 Combat Move units with a double click - units are truncated & ... #12447

TheDog-GH opened this issue Mar 19, 2024 · 25 comments
Labels
2.6 Feature Request Problem A problem, bug, defect - something to fix

Comments

@TheDog-GH
Copy link
Contributor

TheDog-GH commented Mar 19, 2024

Say using 1941 Global Command Decision.
On Combat move
Double click on Paris you get this, note 5 units are displayed
image

.
Using the Territory tab on the right panel you get this, , note 5 units are displayed +2 non-moving with a scroll bar
image
image

.
Observations - The Double click combat move;

  • units are truncated vertical and horizontally and the order differs from the other image.
  • It does not have a scroll bar
  • Maybe the display code can be reused/shared

.
Here is 113 B Sea Zone (West of Denmark) for comparison, the ships can be about 100px wide
image

@TheDog-GH TheDog-GH added the Problem A problem, bug, defect - something to fix label Mar 19, 2024
@asvitkine asvitkine added the 2.6 label Apr 21, 2024
@frigoref
Copy link
Member

Could be related to the fact that the image icons are not squared
image
in comparison to for example World War II Global 1940
image

Could someone confirm that it would be a feature request to allow non-squared icons?

Files I found that are related to this:

  • UnitChooser.java method loadNonWithdrawableImage (UnitChooser added via call from MovePanel.java)
    The code works works against height and I think thereby assuming squared icons (same pixels in height and width).
private void loadNonWithdrawableImage() {
      final int unitImageHeight = unitImageFactory.getUnitImageHeight();
      final double nonWithdrawableImageHeight = getNonWithdrawableImageHeight(unitImageHeight);
      nonWithdrawableImage = loadNonWithdrawableImage((int) nonWithdrawableImageHeight);
      final double scaleChange =
          (double) nonWithdrawableImage.getHeight() / nonWithdrawableImageHeight;
      // scaleChange is the factor by which the scale factor of this.unitImageFactory
      // must be larger than the scale factor of uiContext.getUnitImageFactory()
      // so the non-withdrawable image has the right size in comparison to the unit icon.

      final double scaleFactor = scaleChange * unitImageFactory.getScaleFactor();
      // With this scaleFactor the unit images will have the correct height in relation
      // to the non-withdrawable image.
      // The relation is controlled by getNonWithdrawableImageHeight. By the time of writing
      // this, getNonWithdrawableImageHeight makes the non-withdrawable image half as high as
      // the unit images.
      // At the time of writing this, there are two variants of the non-withdrawable image
      // being 24 pixels resp. 32 pixels high.
      // So the unit image will be either 48 pixels or 32 pixels high.

@Cernelius
Copy link
Contributor

Wow @frigoref is back.

About your query, I don't remember I've ever read from anything or anybody that TripleA units are meant to be square (have same height and width), which implies that either

  1. I've overlooked something.
  2. Our map-making instructions are very bad for not letting map-makers know that.
  3. There is no such requirement.

I'm fairly certain that number 3 is the right answer in the moment that, in map.properties, you do define such dimensions singularly (and I would rather expect a single entry if they would have to have the same value).
https://github.com/triplea-maps/the_pact_of_steel/blob/be0efc7042287dd24a2bbb57e5bc01f4fe4fe5d7/map/map.properties#L30C1-L32C16

@TheDog-GH
Copy link
Contributor Author

I would like it to be a feature request. As people buy 2 & 4K screens TripleA limitations are now beginning to show after all this time, (14 years?)

I know that the UI was really designed for 48x48px units with the above in mind maybe 256x256px could be the new unit standard or just dynamic?

@frigoref
Copy link
Member

@Cernelius thank you 👍
The map Pact of Steel file map.properties file shows

#units.width and units.height should exactly equal the size of the unit art images in the unit folder
units.width=48
units.height=48

while for the map 1941 Global Command Decision the map.properties shows

# for place.txt    1  60x55-Land
# for place.txt    1 100x75-SZ
# map values below 1  60x54
units.scale=1
units.width=60
units.height=54

It does not state anywhere that the assumption is height = width and also think this assumption at least should not exist in the code, but maybe that all unit icons have the same dimension?
Also in this case it should be explained somewhere for the map makers and we would need to check the behavior after a map correction.

@TheDog-GH Let's first find out whether this ticket here is a feature request or a bug before labeling it.

@Cernelius
Copy link
Contributor

It does not state anywhere that the assumption is height = width and also think this assumption at least should not exist in the code, but maybe that all unit icons have the same dimension?

I think it is quite clear and obvious that them all should have the same dimensions because each of them should have the dimensions which are defined in map.properties (or else the default, which I guess is 48 in both cases), both for what is written in the documentation and because the placements are fixed and obtained with utilities clearly assuming same dimensions for all units' images of the same map (also considering that they are drawn from their corners, so their actual centres would be increasingly displaced as dimensions differ: if I have all 48x48 pixels units which are drawn from top-left and add a 64x64 pixels unit, the centre of this unit would be closer to the units on its right and bottom and farther from the units on its left and top (this is to show that supporting different dimensions would also need to address this matter of TripleA drawing images from corners according to fixed coordinates obtained from utilities which assume all units' images being of the same dimensions)).

Nevertheless, TripleA features a lot of maps each having units' images which do not have the same dimensions. I do believe that is bad map-making but cannot state it for sure.

I also believe that (unless I'm overlooking something) there is no map-making documentation telling what the maximum units dimensions are, so I would argue that whatever dimensions are currently supposed to be supported, so I would argue that not being the case is a problem rather than a feature request, which can be fixed by either supporting whatever dimensions or somehow officially documenting (letting map-makers know) what the maximum officially supported dimensions are. However, in this last case, I would strongly suggest letting map-makers exceed these dimensions (by accepting the issues related to doing so). However, in this last case, I'm not sure there might be some documentation somewhere about maximum dimensions for units actually being 48 pixels on both axis. My idea is that TripleA was originally developed on that concept, but I don't know if it has ever been officially documented for map-makers to know.

So I would say that

  • supporting different width and height for all images of the same map is a problem fix.
  • supporting different dimensions for width and for height across the images of the same map is a feature request (which to be polished would also require the program to handle placement coordinates in a different (more dynamic) way than it does now).
  • supporting dimensions over a limit may be a feature request or a problem fix: this would need research also to determine if the implied current limit would be 48 pixels on both axis or what else (I would go with it being a problem fix anyway because the documentation needs to clarify it unless I'm overlooking something, in which case the fix would be to write or re-write the documentation so to let map-makers know what the limits are without them having to test them pixel per pixel to find out at what point the image starts to be cut).

Regarding the first and last points, a test map I would suggest is
https://github.com/triplea-maps/conquest_of_the_world
(All units are width=256 height=192.)


I know that the UI was really designed for 48x48px units with the above in mind maybe 256x256px could be the new unit standard or just dynamic?

I do believe that currently TripleA does not feature any images higher or wider than 256 pixels, so that limit would support everything which is currently existing within the TripleA maps' repository.

@Cernelius
Copy link
Contributor

Cernelius commented Jun 30, 2024

Could be related to the fact that the image icons are not squared image in comparison to for example World War II Global 1940 image

My understanding is that this is bad map-making because all those units images should have the same dimensions. If no unit is wider than 96 nor higher than 68, then the most obvious fix is to reset the canvas' size of all images to be 96 per 68 pixels. Doing so with no off-set would keep the current visual, whereas centring the images (adding an off-set equal to half the difference of the dimensions after-and-before on the respective axis) would likely improve the visual. Obviously, units.width and units.height for that map should be re-set respectively to 96 and 68, and the place.txt file should be re-made accordingly.

If it is wanted to keep the map as it is, however, I would say that would make the map experimental and in need of one or more additional features (to be requested) to support differently sized images for the same map. However, that would not entail just avoid cutting bigger images but also re-defining at the root the way placement coordinates are generated and handled in TripleA, so for example to allow placing a big unit in a zone together with smaller units in a polished way. I do believe such a feature would be fairly popular.

@TheDog-GH
Copy link
Contributor Author

Maybe relevant, currently the Battle Calculator appears to be limited to 54px high.

@frigoref
Copy link
Member

frigoref commented Jun 30, 2024

Let me summarize our discussion so far - at least how I understood the posts above:

  1. In the map specific file map.properties there are entries for unit.height and unit.width. The meaning is not clear yet, but the assumption is that all unit icon dimensions should match those values to allow a proper display in triplea.
    -> Anyone with more information on the existing design and design assumptions is welcome to enlighten the rest of us.
  2. Even though originally only squared unit icons (48x48) seemed to have been used, our assumption is that unit icons is the same rectangle shapes should be supported.
  3. The map for which this issue was opened (1941 Global Command Decision) does not fulfill points 1. and 2., thereby disqualifying at the moment as a bug example.
  4. Map conquest_of_the_world fulfills points 1. and 2. (all units are width=256 height=192.) and should be analyzed for the above mentioned truncation of images.
    -> Who can take this up? (@Cernelius Thanks for finding this map, would you be willing to take this one up?)
  5. The order of the units differs in the popup from the tab Territory on the right. They should be in the same order.
    -> I can take this up and check for a code fix.
  6. The popup does not have a scroll bar.
    -> @TheDog-GH: I cannot see why it is needed as long as you can see everything. Was this supposed to be a bug you wanted to report or is it just an observation?
  7. The display code cannot be reused from the tab Territory within the popup as the popup shows scaled images without flags for the purpose of reducing the popup size and is thereby different.
  8. To support different unit icons dimensions in a map is a feature request which would need to be stated clearly (how to handle scaling at different places, etc.) and is not part of this issue.
    I deduce this from nobody being able to state how the code should handle that and the code does not show any indication on this being tried to account for.
  9. There might be a maximum for the unit icon dimensions.
    Not sure how @TheDog-GH deduces

currently the Battle Calculator appears to be limited to 54px high
-> See point 1: Anyone with more information?

  1. There is a need to collect all limitations / features currently supported related to unit icon dimension and make the transparent for map makers as well as programmers (e.g. by code comments).
    -> See point 1: Anyone with more information?
    -> Could someone find suitable places where to put the information once we have it?

I would like to focus on bug fixing code changes, but in order to do that I need clear bug reports preferably without "I would think the code should"-statements. For anyone interested, please checkout https://testlio.com/blog/the-ideal-bug-report/
Anything not related to bugs should be moved to a separate ticket once we have clarified the details of the bug for this issue ticket.

@Cernelius
Copy link
Contributor

I suppose I can try to reproduce with "Conquest of the World", but first I would need to know what is the problem with the Territory tab. Am I to assume that the Territory tab has been displayed merely to point out the different units' orders, and it actually has no problems of its own?

(For off-topic information, "Conquest of the World" is an experimental map that is just there waiting for a developer to turn it into something like "Risk" by supporting such a rules-set in TripleA.)

@frigoref
Copy link
Member

frigoref commented Jul 1, 2024

@Cernelius This ticket is about 2 issues: First truncation of the unit icons in the popup and second the different order of their display (currently I do not see an alignment on the "correct" order that is to be expected).

@TheDog-GH
Copy link
Contributor Author

I am on holiday, so cannot interact properly for 7 days

@Cernelius
Copy link
Contributor

Cernelius commented Jul 2, 2024

Say using 1941 Global Command Decision. On Combat move Double click on Paris you get this, note 5 units are displayed image

There is not this issue with Conquest of the World.

I believe that the issue is due to the fact that the truncated units are bigger than the dimensions defined in map.properties, and I consider this to be a map rather than program problem (unless it is intended for the map to be experimental for a feature request).

To the author of the map, I would ask why he/she did not give the same dimensions to all units' images (just giving additional transparent areas to the smaller ones to bring them up to size). Was it because overlapping was wanted, but the map-making utilities do not allow it?

I close this issue as invalid and suggest opening one or more new ones, each of them for any outstanding item, either here or in the repository of the problematic map (but please just reopen it if anyone thinks it should not have been closed as I'm not sure myself).

(@DanVanAtta Are map problems (problems which require changes to one or more maps rather than to the program) to be tracked in this repository?)

@Cernelius Cernelius closed this as not planned Won't fix, can't repro, duplicate, stale Jul 2, 2024
@frigoref
Copy link
Member

frigoref commented Jul 2, 2024

@Cernelius I agree with you on the topic of image truncation / unit icon dimensions.
Before closing this ticket, we can still:

  • Recheck with another map this assumption:
  1. Map conquest_of_the_world fulfills points 1. and 2. (all units are width=256 height=192.) and should be analyzed for the above mentioned truncation of images.
    -> Who can take this up? (@Cernelius Thanks for finding this map, would you be willing to take this one up?)
  • Align the order of unit icons displayed between popup and tab Territory:
  1. The order of the units differs in the popup from the tab Territory on the right. They should be in the same order.
    -> I can take this up and check for a code fix.
  • Make the requirement to the unit icon dimensions better know to map makers:
  1. There is a need to collect all limitations / features currently supported related to unit icon dimension and make the transparent for map makers as well as programmers (e.g. by code comments).
    -> See point 1: Anyone with more information?
    -> Could someone find suitable places where to put the information once we have it?

Could you do 4. and 10. while I handle 5. ?

@Cernelius
Copy link
Contributor

Could you do 4. and 10. while I handle 5. ?

Sorry, but I cannot think of an other thus properly made map with "about 100px wide" or wider units, and I'm not sure how to handle 10. I would say that would be better handled as a discussion ticket for it only to start thinking about what and where has to be done.

However, regarding point 1, I see it as being already well enough covered because I think that "The Pact of Steel" map already substantially states it.

https://github.com/triplea-maps/the_pact_of_steel/blob/be0efc7042287dd24a2bbb57e5bc01f4fe4fe5d7/map/map.properties#L30C1-L32C16

#units.width and units.height should exactly equal the size of the unit art images in the unit folder
units.width=48
units.height=48

Is it really that unclear that stating that "units.width and units.height should exactly equal the size of the unit art images in the unit folder" also means that the width and height of every unit art image in the unit folder should be equal to respectively the units.width and units.height? How much "for dummies" does the documentation need to be?

For the remaining parts, I think that I'm in need of clarifications just as much as anyone else in here and likely more so than a developer who can read the code and have a better guess if there are any maximum limits to units' images' dimensions.

To be clear, I've no programming skills.

Also, I would basically need to re-read all the map-making instructions to make sure we are not missing something, and this would take some considerable time.

I'm available for updating "The Pact of Steel" if anyone can clarify anything about maximum dimensions and such. I'm thinking at something like "these are the maximum dimensions, but you are allowed to go beyond them, but be warned there will be display issues if you do so" or "whaver dimensions are supposed to be supported, but be warned that there are display issues overe these dimensions which have not been yet fixed". However, no such display issues have emerged for what reported at the opening post of this issue when testing with "Conquest of the World".

Let me know if I'm overlooking something.


Should this issue be re-opened? Is it wanted to keep closed issues in Back Log?

@Cernelius
Copy link
Contributor

I don't know if it is somehow possible to make a mass search in the entire "maps" repository for every map having in its map.properties units.width=x with "x" equal or bigger than 100. One of those maps, beside "Conquest of the World", could then be used to double check what I tested with "Conquest of the World".

@frigoref
Copy link
Member

frigoref commented Jul 3, 2024

@Cernelius I wrote a sidemap for a Chrome web scraper to extract this information and found:

  • Most maps do not have these parameters (units.width and units.height) specified in the file maps.properties
  • Maximum values are units.width=256 and units.height=192 for map Conquest of the World and file maps.properties
  • OBSOLETE: The units German battleship (battleship.png 55x55) and German airfield rocket (airfield_rockets_r.png 56x48) differ
  • Most values are maintained with <100

You can also check the extraction file TripleA_Maps.xlsx generated with the following sitemap:
{"_id":"TripleA_Maps","startUrl":["https://github.com/orgs/triplea-maps/repositories?page=1"],"selectors":[{"id":"mapLink","parentSelectors":["_root","NextPage"],"type":"SelectorLink","selector":"a.iPboZF","multiple":true,"linkType":"linkFromHref"},{"id":"NextPage","parentSelectors":["_root","NextPage"],"paginationType":"clickMore","type":"SelectorPagination","selector":"a[aria-label='Next Page']"},{"id":"mapFolder","parentSelectors":["mapLink"],"type":"SelectorLink","selector":"a.Link--primary[title~=\"map\"]","multiple":false,"linkType":"linkFromHref"},{"id":"propertiesFile","parentSelectors":["mapFolder","mapLink"],"type":"SelectorLink","selector":"a.Link--primary[title~=\"map.properties\"]","multiple":false,"linkType":"linkFromHref"},{"id":"textFull","parentSelectors":["propertiesFile"],"type":"SelectorText","selector":"textarea.react-blob-textarea:contains(\"units.width\")","multiple":false,"regex":"units.width=[0-9]+\\nunits.height=[0-9]+"}]}

OBSOLETE: Could you try with the list to find another map that has larger unit dimensions which are consistent in the images provided and check whether you can see any display issue?
If not, I would be also fine with leaving point 4. for the time being as this reported issue can be explained with the differing unit dimensions.

The would leave point 10. for you to find a proper place where to state the requirement to have the unit dimensions the same for all units.

@Cernelius
Copy link
Contributor

@Cernelius I wrote a sidemap for a Chrome web scraper to extract this information and found:

  • Most maps do not have these parameters (units.width and units.height) specified in the file maps.properties

Actually, this would need documentation too (about what happens when you don't have them). Can anyone confirm that what happens is that they are both set at 48 (practically not having them being the same as having them at 48 each). "The Pact of Steel" seem to imply that you must set both of those values, and you are telling me that this is practically not true, so I would say this is an other documentation problem unless the documentation is correct, and maps which fail to set them are to be considered supported but deprecated?

"World War II Pacific" does not have any setting for these dimensions. I don't know what you are seeing there being 256 pixels per 192 pixels.

I don't understand what you are getting at here. I'm guessing the rest of the units are 48x48 because the values in map.properties are missing, or not?

  • Most values are maintained with <100

Looks like "Conquest of the World" is indeed the only properly made map exceeding 99 pixel on either axis.

You can also check the extraction file TripleA_Maps.xlsx generated with the following sitemap: {"_id":"TripleA_Maps","startUrl":["https://github.com/orgs/triplea-maps/repositories?page=1"],"selectors":[{"id":"mapLink","parentSelectors":["_root","NextPage"],"type":"SelectorLink","selector":"a.iPboZF","multiple":true,"linkType":"linkFromHref"},{"id":"NextPage","parentSelectors":["_root","NextPage"],"paginationType":"clickMore","type":"SelectorPagination","selector":"a[aria-label='Next Page']"},{"id":"mapFolder","parentSelectors":["mapLink"],"type":"SelectorLink","selector":"a.Link--primary[title~=\"map\"]","multiple":false,"linkType":"linkFromHref"},{"id":"propertiesFile","parentSelectors":["mapFolder","mapLink"],"type":"SelectorLink","selector":"a.Link--primary[title~=\"map.properties\"]","multiple":false,"linkType":"linkFromHref"},{"id":"textFull","parentSelectors":["propertiesFile"],"type":"SelectorText","selector":"textarea.react-blob-textarea:contains(\"units.width\")","multiple":false,"regex":"units.width=[0-9]+\\nunits.height=[0-9]+"}]}

Actually, looks like that there is an other map exceeding 99 pixels on either axis: "Elemental Forces" at 120 per 120.

Could you try with the list to find another map that has larger unit dimensions which are consistent in the images provided and check whether you can see any display issue? If not, I would be also fine with leaving point 4. for the time being as this reported issue can be explained with the differing unit dimensions.

Done with "Elemental Forces", but I also think that the testing done with "Conquest of the World" should be enough and we can consider point 4 done anyway.

The would leave point 10. for you to find a proper place where to state the requirement to have the unit dimensions the same for all units.

I guess that means that the current explanation in "The Pact of Steel" is not sufficiently clear? Well, my knowledge of that requirement comes from that explanation only, but I guess I can reword it to be clearer while maintaining its substance as it is.

@frigoref
Copy link
Member

frigoref commented Jul 4, 2024

@Cernelius Sorry, I mixed up two line and therefore the previous post was not correct. I have adjusted it.
Could you test map Conquest of the World? The unit dimensions seem to be consistent at a first glance (checked blue army).

@Cernelius
Copy link
Contributor

@Cernelius Sorry, I mixed up two line and therefore the previous post was not correct. I have adjusted it. Could you test map Conquest of the World? The unit dimensions seem to be consistent at a first glance (checked blue army).

I don't understand what you want me to test because I've already tested that there is no truncation in "Conquest of the World" and told so in the post where I said

There is not this issue with Conquest of the World.

Can you tell me what needs testing?

However, the units are zoomed to make them small enough in the prompt, whereas I would argue that it should rather be the prompt to be big enough and allocate enough space to display the units at their full sizes.

@frigoref
Copy link
Member

frigoref commented Jul 4, 2024

@Cernelius Sorry again, I must have forgotten about this. Then this is done and the only thing open for this issue ticket is point 5.

  1. The order of the units differs in the popup from the tab Territory on the right. They should be in the same order.

@Cernelius
Copy link
Contributor

Actually, there are possibly still the changes and additions to "The Pact of Steel" which we have discussed, though here we would need someone with the authority to state what is what. I guess it should be the owner or the owners of the repository.

@Cernelius
Copy link
Contributor

To clarify, these are at least some of the items about documentation.

  • Have someone confirm that, when units.width and units.height are missing, each of them defaults to 48, then document this.
  • Decide whether or not one should always set units.width and units.height. If yes, clarify that not having them is supported but deprecated (as the documentation appears to state). If no, reword the documentation as to state that you need to set them only if they are not the default (to be documented).
  • Decide if there is a need to be clearer about all images having to have the same dimensions within the same map, and re-write the documentation accordingly if so.
  • Check if there is any sort of maximum limit to units.width and units.height and document this limit while also deciding if this is a wanted limit or not. If yes, decide and document what is what in case any map-maker decides to go past it (assuming this is not a hard limit, so maps exceeding it are still acceptable despite having display issues). If no, document that every dimension is meant to be supported (as it appears to be the case currently because the documentation tells you that you can set such dimensions without giving any limits), so whatever issue for units being too big is to be considered a pending problem.

@TheDog-GH
Copy link
Contributor Author

@frigoref & @Cernelius
I had these setting in the map.properties
units.width=60
units.height=54

After some testing I have changed to these
units.width=98
units.height=68

They are the width and height of different units
The Carrier-Fleet ship is 98 width
The German Navy Commander is 68 high

image

So for me this is part is fixed,, but I will do more testing to see if these numbers work as expected/intended.
Thanks both for helping to resolve this.

.
In the image below the Move panel unit order is the same as the Status bar unit order (with addition of non-moving units, so this is good.

However the Territory tab unit order (far right) does not match, so this is probably using a different data table to display this list and is a waste of memory, yes? Should it not use the same table as the other two?

image

@frigoref
Copy link
Member

frigoref commented Jul 9, 2024

@TheDog-GH For the UnitChooser popup there is currently no sorting implemented as it is for the tab Territory and likely also for the MovePanel (I will have to check the last one as well).
My idea is to let all use the same sorting if possible. Will create a PR later this week I think and update this ticket.
Just FYI: There is always a new list used and as the system is using references (instead of full copies) the memory impact is negligible.

@TheDog-GH
Copy link
Contributor Author

@frigoref
The right hand panel order appears to be in the order of the paired coordinates in the place.txt file.

I have just been manually micro adjusting the co-ordinates for the TT Paris in the place.txt file.
So if you want an ordered list this could be the one to use?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.6 Feature Request Problem A problem, bug, defect - something to fix
Projects
Status: Back Log
Development

No branches or pull requests

4 participants