triplea-game / triplea

TripleA is a turn based strategy game and board game engine, similar to Axis & Allies or Risk.
https://triplea-game.org/
GNU General Public License v3.0
1.32k stars 392 forks source link

Revert territoryEffects Bottom Bar Display Request (1.9.0.0.10186) #3396

Closed Cernelius closed 5 years ago

Cernelius commented 6 years ago

I believe you made a change that should be reverted.

I see that the image in the territoryEffects folder now displays in the bottom bar, similarly to the images in resources.

However, differently from resources, territoryEffects was not an unimplemented item, but it is implemented as displaying those images on board, by coordinates defined in territory_effects.txt.

Practically, the territoryEffecs folder was akin to the PUs folder, not to the resources folder (and I actually believe you want to have much bigger images for the territory effects than for the production values, on board).

It cannot be sensibly used to do both, as on the board you normally want to display images much bigger than what you get in the bottom bar (this is why you have, for example, "large" and "small" versions for flags).

For example, in a map I'm making, I'm using this territory effect image (that displays on board):

conquered

And now I see this display in the bottom bar:

20180424_territoryeffects

So, I request this change be reverted, going back displaying just the name of the territory effect, in the bottom bar, not the image in territoryEffects, which has always been purposed to display on board.

p.s.: Having an image display for territory effects when referenced outside the board is a good idea, and I would open a feature request for that (likely to be done similarly to the "flags" folder). I'm just saying taking the current images in territoryEffects is not the right way to do it (like if you would use the board display "large" flag in the bottom bar).

p.p.s.: Alternatively to reverting, I would request having "X_large" versions in territoryEffects, for board display, by territory_effects.txt, where "X" is the name of the territory effect (whose standard image would still display, instead, in the bottom bar).


Fix


TODO for Next Stable Release There are 2 maps in the repo that map.useTerritoryEffectMarkers=true and I'll be updating those when we push the next stable release so their images are _large.png: another_world: triplea-maps/another_world#2 stellar_forces: triplea-maps/stellar_forces#3

Cernelius commented 6 years ago

I also see you have removed the "territoryEffects" folder from the current TripleA assets (not requesting to revert this removal). In 1.8.0.9, it had images spanning from 63x41 to 74x37 to 57x46, clearly not to be used for the bottom bar. Just for reference.

ron-murhammer commented 6 years ago

@Cernelius How do territory effects used to display on the map differ from decorations?

Cernelius commented 6 years ago

Decorations are images that you place on the map directly, as skin only elements. Territory effects are images that are placed based on the territory having that territory effect in the xml of the specific game (exactly like the large flag images for the capital territories or the PUs images for any territories).

Anyways, the matter is not the territory effect system, that works fine, but the fact that you used those same images for bottom bar display. I think it should be clear that a same image can hardly be optimized for both uses (or, anyways, this is what I believe).

So, I'm requesting you to stop/revert using the territory effects images for bottom bar display (I assume this is a recent new feature?).

Having different versions for board and bottom bar display, like in the case of the flags folder (probably here 2 versions are enough, instead of the 3 versions for the flags) may be a good feature request, alternatively to reverting.

Cernelius commented 6 years ago

To be clear, this is not a feature request. I'm requesting you to stop using those images for bottom bar display. The other suggestions are optional (we can open a feature request in forum after this revert is done).

Cernelius commented 6 years ago

In 1.8.0.9 the territoryEffecs images were for board display only (like the PUs), as far as I know.

DanVanAtta commented 6 years ago

@Cernelius it's not the best request to say "undo this!" Technically with this much time passed it's not a trivial exercise.

Instead I think we should focus on the problem you've found, mark this issue as a bug, and see what solutions we can reach without necessarily removing functionality that is really useful for some maps, but does not necessarily work well with some of the image sizes (which really has this fitting the definition of a bug).

It seems like the problem is that territory effects are not scaled correctly, if they were scaled correctly, would that adequately fix this problem @Cernelius ?

Cernelius commented 6 years ago

I guess having the image in the bottom bar resizing properly if bigger may be fine enough. I still believe having separate images, like in flags, would be preferable.

Cernelius commented 6 years ago

For a test map, you can use the recent "Another World" by @BeornTheBold.

Heppisorus commented 6 years ago

I think there is a feasible work around while designing the map....but having 2 defined territory effect images (one for the menu and one for the map) would be a neat feature. Though really the original intent for the territory effects was more or less useless from a graphical standpoint on the map.

Cernelius commented 6 years ago

If I can give labelling suggestions, I ask this issue to be labelled as regression, instead of discussion.

In 1.8.0.9 you could have had big territory effects images and the effect clearly explained in the bottom bar. Now, if your board image is too big, the bottom bar info you are receiving may be close to meaningless.

With regression I don't mean (and I don't believe) that the matter is important. As long as the main existent feature to draw it on the board is there and works well (and it does), I factually never go looking at the bottom bar.

DanVanAtta commented 5 years ago

Revisting this, @Cernelius it looks like the image is simply being cropped. What if it were properly resized?

Cernelius commented 5 years ago

Revisting this, @Cernelius it looks like the image is simply being cropped. What if it were properly resized?

That would be better, and would substantially solve the problem, as long as the zoom is decent.

Of course, having different images tailored for the different places, just like it works for the capitals (where you have the "large" one for the board, the "small" one for bottom bar (not that it has to be large or small, that is up to the mapmaker)), would be better.

Cernelius commented 5 years ago

If you want to test it specifically, this is the image I'm using (128x128 pixels):

conquered

ron-murhammer commented 5 years ago

To address this request, separate images for displaying territory effects in the bottom bar and on the map should be added. Resizing is generally not going to address this well and if you do want that temporary solution then it should only be used for territory effect images and only if they are over a certain size (not always resizing to a certain size).

This shouldn't change how resource icons work. We need to revert #4623 and look to address this in a better way.

ron-murhammer commented 5 years ago

@Cernelius Do you know what maps in the repo use this to place territory effect images on the map?

Cernelius commented 5 years ago

@ron-murhammer I'm not aware of other maps using this feature. I would need to download, then unzip, then local search all the maps, and even this way I would just know what of download list, not the whole repository. Is there a way to just search into the repository directly?

Anyways, I believe territory effects images on the map is an important feature, despite being very rarely used.

I still believe integrally reverting the display would be a good move, as, that way, you see the territory effects on the board and get their names in the bottom bar (differently from the resources, that are metadata, not normally visualized).

FrostionAAA commented 5 years ago

I have just noticed this work and this thread as I downloaded and played with the latest pre-releases.

The new displaying of icons seem to resize the resource icons in the lower bar of the map and also in the purchase screens. And it looks awfull. The zoom really destroys the icons. Some icons, meant to be 20x20 px, are streached to be 30x30 px it seems.

The icons I have made for maps were never meant to be resized. On the contrary, every mini picture I have made for my maps have been specially constructed, not just a small version of a big picture. For example the big version of a picture used in relation to a map might have a small counterpart in size, but they both might still have a similar black outline border of 1 or 2 px. So personally I can’t see it as practical to use picture zoom at all.

Also some icons have proportions in height and width that have been adjusted in comparison to other icons, so that they might seem to fit each other in size/mass/style, not just the same height. Some icons might be wide and others might be high, to degrees that make them seem in line with each other. They are meant to be displayed in true size pixel for pixel, not stretched to all be the same height. I don’t see any other practical solution than have the game support different image sizes, just like in the resource directory, flags and such.

DanVanAtta commented 5 years ago

This is a good example where we needed the standard blurb of an issue to be on the same page for what the problem was and how to fix it. Going forward bug reports that skip over the standard questions could be closed.

Let's please be sure to also all stop collectively using the word "revert", it is loaded and a bit meaningless when talking about bug reports.

Adding standard information, I think the following is accurate, please inform if incorrect:

Map name

Total world war december, or any that uses territory effects and has images that are larger than 32x32

Can you describe how to trigger the error? (eg: what sequence of actions will recreate it?)

Swap out a territory image for a large icon, then hover over the territory and notice the image in the window menu bar.

Do you have the exact error text? Please copy/paste if so

Bad rendering: 20180424_territoryeffects

Instead of this error, what should have happened?

Display text of the territory effect instead of the image


Is the above desired fix accurate? To go to territory modifier text instead of an image?

ron-murhammer commented 5 years ago

@DanVanAtta The original bug actually doesn't impact TWW since it only uses territory effect images in the bottom bar not on the map so they are already appropriately sized for that so that map description is misleading. The original bug report impacts any map that was using territory effect images on the map territories that were not intended for the bottom bar.

After your original PR to address this now we have several other issues around resizing images being blurry, resizing images that shouldn't be resized, and resizing resource images that had nothing to do with this bug.

I added this to the first post already but here is what needs done to address the original bug and the bugs introduced in the PR:

TODO

DanVanAtta commented 5 years ago
* To address this request, separate images for displaying territory effects in the bottom bar and on the map should be added.

I'm not sure I agree with this, this is a large update and adding duplicate images of different sizes in general I think is very bad. Any image that is going to look bad resized would also have been clipped and looked bad anyways. It seems we are saying the original bug is that we need a new feature to allow for multiple image sizes. It seems like we should go back to text until we have the feature in place.

DanVanAtta commented 5 years ago

I think the TODO list should be as follows:

DanVanAtta commented 5 years ago

@ron-murhammer it's easy to of course to drop the resize code or add conditionals to it. It seems pretty clear we should have matching images for territory effects rather than image in one place and text in another.

There are other reasons beyond code complexity/maintenance to not desire a second image, it's another thing for map makers to deal update if they update images and adds more download weight to the map / repository.

Cernelius commented 5 years ago

I agree with the TODO edited as my first post. But you should also decide, and document, how you are going to differentiate the images (by naming, I suppose).

Despite the fact that this feature was originally supported for board display only, I suggest the image for board display having to end in "_large", while the one for bottom bar etc. having no suffix (as per current), to be coherent with how flags work. If the "large" version is missing, the regular one should be used in its stead.

Can we make it so by default only one image is needed with an option to give a second?

Under my suggestion, this would be covered by not having the large version.

The resize of large->small seemed to have looked good, is perhaps the answer to just only resize to shrink an image? If it's the case that we can't get it to look good, then IMO we would add an optional icon sized image.

Resizing should be good enough, as long as it only shrinks images that are too big for the available space (instead of cropping them), leaving the rest alone. For example, the space in the bottom bar is given by how tall the regular flag image is.

DanVanAtta commented 5 years ago

Resizing should be good enough

This makes sense, I think we are coming to agreement. if the shrink to icon size does not look good then an icon specific image can be provided. From that perspective a "_icon" image would be used if present, otherwise shrink-if-needed the current image to 32x32 (the code currently has 32x32 as the target size)

ron-murhammer commented 5 years ago

4679 should address this issue. It addresses it pretty much as @Cernelius described above minus not doing any resizing (could be added in the future but not really necessary). There are now 2 territory effect image options: \<effect>.png (existing) and \<effect>_large.png (new).

The bottom bar only uses \<effect>.png if not present then uses name.

The map territory effects only show if map.useTerritoryEffectMarkers=true and first uses \<effect>_large.png if that isn't found then tries \<effect>.png.

Here is an update to POS2 to add description for useTerritoryEffectMarkers: https://github.com/triplea-maps/the_pact_of_steel/pull/28/files

@Cernelius Given this change you should update your in-progress map to use \<effect>_large.png if you want name to be shown in the bottom bar.

@FrostionAAA Once that PR is merged, the resource images in all your maps should be fixed and display like they do in the current 1.9 stable.


TODO for Next Stable Release There are 2 maps in the repo that map.useTerritoryEffectMarkers=true and I'll be updating those when we push the next stable release so their images are \<effect>_large.png: another_world: https://github.com/triplea-maps/another_world/pull/2 stellar_forces: https://github.com/triplea-maps/stellar_forces/pull/3

Cernelius commented 5 years ago

The only dubious part is the feature of using the regular image if the large one is missing.

This is inconsistent with how flags work, as in that case you will error out if the needed flag is missing.

Also, this may bury mapmaking mistakes, in that the mapmaker missed or wrongly named the large flag, but the game still works on the regular one.

I would personally prefer to keep this behaviour alike to the flags, in that only the large version is called for the board (I just advanced that suggestion for backward compatibility and since there was the request of having the single image option).

FrostionAAA commented 5 years ago

Yes, it would be preferable to have multiple territory effect images, just like flags and resources, or at least the option to use different images if the mapmaker does not see it fitting to just use a shrunken down version of the large one.

From a mapmaker perspective it would also allow the menu icon to vary a bit and not be the exact same as the board's graphical representation. Graphics on the board might be more detailed and the icon needed at the bottom might need to be simplified or in other ways deviate from the on on the board.

Also, the file size of a few small icons would be insignificant in regards to upload/download I think.

Cernelius commented 5 years ago

From a mapmaker perspective it would also allow the menu icon to vary a bit and not be the exact same as the board's graphical representation. Graphics on the board might be more detailed and the icon needed at the bottom might need to be simplified or in other ways deviate from the on on the board.

@FrostionAAA I wonder if you (or me?) are misunderstanding the matter now, as the whole point of this discussion, since its start, was to have different images for board and bottom bar (like using a 128x128 pixels image on board, without having a bottom bar 128 pixels tall). So what @ron-murhammer is already doing is having different images for these two cases. Or you mean something else, with "menu icon"?

FrostionAAA commented 5 years ago

@Cernelius What I am saying is, that engine downscaling or upscaling of an image (like your example for territory effects) does not always lead to good or wanted results, so there should be an option open to use images specially made for display at the bottom bar be displayed at the bottom bar, as well as use images meant to be displayed on map be displayed only on the map.

Any decision now or in the future, that would be based on the assumption that images do not need to exist duplicated / exist in more that one big version, thinking that the one big version could just be scaled down to a smaller size, would be wrong.

It's not just a matter of pixel scale. There are other factors in play, as I have already mentioned.

DanVanAtta commented 5 years ago

What I am saying is, that engine downscaling or upscaling of an image (like your example for territory effects) does not always lead to good or wanted results,

I think we all agree. I am suggesting the engine never upscale, and downscale only if there is a '_large' image presenet. @ron-murhammer is proposing to remove scaling and go back to cropping, which IMO is essentially the problem originally reported. In general, if we solve image size problems by requiring a version of the image at each size, we are forcing this on everyone, even prototype map makers or those who have images that can be scaled well. I'm strongly of the opinion the game engine should have automation to use only one image copy and provide the option for displaying multiple versions. If we take this approach too frequently then we are multiplying the necessary images for any given map.

DanVanAtta commented 5 years ago

https://github.com/triplea-game/triplea/pull/4679 was just merged, that puts us back to the original problem with a new workaround of providing an optional second image.

To do shrinking properly we need a deeper change compared to #4623 as the drawing code is shared.

ron-murhammer commented 5 years ago

@DanVanAtta I think this can be closed unless the current pre-release doesn't address @Cernelius and @FrostionAAA concerns.

The issue was that the previous changes had caused territory effect images only intended to be displayed on the map and also showing them in the bottom bar. This is fixed now by using _large.png and aligns with how resource and flag images work. Any additional improvements around down scaling if too large should be a new feature request and probably address all image types consistently.

DanVanAtta commented 5 years ago

I agree we should be consistent about an optional image and scaling. Having a workaround in place is fine for closing an issue. I think the "This is not a bug but a feature request and closing without follow-up" should just be avoided.

What @Cernelius was working on looked fine, we switched to images and then it did not. That is the 'regression' vs bug side of the argument here. It's not an interesting debate when arguing between regression vs buggy feature, it's distracting.

Going forward I'd ask we simply not close as a 'feature request' and instead add the label, or just say we have a new workaround and close because of that (which is the case here).

ron-murhammer commented 5 years ago

@DanVanAtta This isn't a workaround... If anything it not only addressed the original issue of reverting the behavior that @Cernelius experienced, it also added the ability to have separate images and aligned the naming to the existing resource/flag images. The original issue report had nothing to do with scaling and that didn't come up until you suggested it. I would really suggest reading the original issue post again.

DanVanAtta commented 5 years ago

If anything it not only addressed the original issue of reverting the behavior that @Cernelius experienced

Wait, the requested revert was to go back to a 'text' display.

So, I request this change be reverted, going back displaying just the name of the territory effect,

If the original problem is reproduced, to my understanding this is rendered:

20180424_territoryeffects

Because the problem is fixed by the map-maker taking action to add images, I've called that a workaround.

ron-murhammer commented 5 years ago

@DanVanAtta Any existing maps that only want territory effect images on the map and text in the bottom bar just need to add "_large" to each image. There isn't any need to add images.

See these: another_world: triplea-maps/another_world#2 stellar_forces: triplea-maps/stellar_forces#3

Cernelius commented 5 years ago

I think this can be closed unless the current pre-release doesn't address @Cernelius and @FrostionAAA concerns.

It does (tested it with 1.10.14240). I actually prefer the 2 images solution to revert. The reason why I asked for revert is that otherwise this would have been a feature request, and just wanted to point out that I believed the current feature was problematic.

Another consistency element, you may want to consider, is that the calling of images should be either ever or never pronted by map.properties entries, rather that it being so in one case (the board) and relying on missing the image in the other one (the bottom bar). Isn't this a case of inconsistent, thus confusing, processing?

I tend to believe that, at this point, if a mapmaker would make an image ending up cropped (like in my example), that should be considered a fault in the map, tho it can be argued that the program should not let that be (but not even sure what would be a good behaviour; I guess the program should throw an exception, telling that the image is being partially cut).

I also advice not making any automatic resizing in general, especially since TripleA is not vector based.

Talking in general, I could agree that, for example for the flags, our current process of having 3 different images for the various places, with no rescaling, is limited (and plainly dumb in the moment the mapmaker just wants one same image at different dimensions), but any better alternatives would require having a vector (not raster) graphic referring image. I believe automatic default resizing is to be reserved for vector graphics only (so, never, in TripleA), as I definitely believe multiple images and no rescaling is always the best (or even mandatory) solution dealing with raster graphic.

Meaning I actually suggest not adding any resizing option/alternative, on top of the current behaviour, assuming TripleA will remain purely raster graphic based.

I cannot see any possible further enhancement here, unless you want to maybe support also small images, for places like the battlecalculator (but I'm not even sure that would be a good idea).

DanVanAtta commented 5 years ago

@Cernelius thanks for the feedback. AFAIK, if an image that is 32x32 is shrunk in half, it should not suffer distortion. Map making is overly complex, having ways to avoid any redundancy/complexity IMO a big win. Having seen image shrink down and look good, I have trouble being convinced that we should force all map makers, every time to always specify copies of an image. When it comes to simplifying the map making process, forcing needless redundancy I feel is a step in the wrong direction.

FrostionAAA commented 5 years ago

I feel that arguments that support high quality mapmaking as well as focus on picture detail is being neglected in this discussion, so I will point out the certain things again, as I fear for the future in regards to the ability to customize and create nice aesthetical graphics.

While it might make sense to have the engine re-scale unit pictures, that is not the case when it comes to stuff like flags and territory effects, where the graphics are used as/in menus as well as on the map. There the _small, normal, _large version are used in different places and the mapmaker CAN wish for different pictures, and not necessarily want the exact same picture just autoscaled up/down. Same goes in regards to resource pictures when one size is used in bottom bar and another at purchase screen (if resources can be purchased). Here the larger unit size picture can be showed with great detail and needs its own picture, while the bottom bar version needs to be free of detail and look good small ... and that is not necessarily and most unlikely just a small version of the large.

Look at Total World War where the _small is a totally different picture than the normal, since the normal is a menu picture and the _small is for unit flags and history. In Iron War, the _large is totally different from the other flags, since it is a 3D picture and the other versions are 2D.

Additional to this, it is not desirable to have mini pictures just be mini versions of something that might look good large. There is perhaps a need for the different version to have stuff like shadows, outlining, details be customised for the specific size... and this is WHEN essentially the same picture is used, meaning that the mapmaker would still need different versions.

So please don't think about limiting the possibilities when it comes to different pictures.

DanVanAtta commented 5 years ago

So please don't think about limiting the possibilities when it comes to different pictures.

Hmm, there seem to be a confusion. Nobody is stating to take anything away. If someone wants to specify N images for different dimensions, it's supported and I've never intentionally suggested that we should drop that support. On the other hand, it does not seem right, in the name of quality, to also force different images when scaling could sometimes do the job. I'm saying we should support an option to have one image and not force N.

DanVanAtta commented 5 years ago

I feel that arguments that support high quality mapmaking as well as focus on picture detail is being neglected in this discussion,

I really disgree with this.

"20x20 px, are streached to be 30x30" will look bad

Yes, that will look bad. But, if you had an image that was 128x128 scaled down to 32x32, it very well could be pixel-perfect compared to the same image at 32x32. In those cases, where that works, forcing people to supply multiple images is adding overhead. I am not suggesting that you only be allowed to supply one image, I'm just saying it should not be the case where you should be forced to supply multiple.

DanVanAtta commented 5 years ago

Question (1): If sizes line up so that pixels are not being divided fractionally, scaling should look good? As coded in one iteration here, it seemed so, but I want to be sure that there is not something that I do not know here.

Question (2): what happens when we find that 32x32 images are too small and want to scale everything up? Same for larger images? Do we then require maps to start adding a new copy of every image?

I think what we should be aiming for is to set a standard for image sizes. In theory we could allow images to be give to us at a higher resolution to be future proofed for the day when we want to make images even larger (say when 8k monitors become common).

This is why I feel so strongly it's the wrong direction to always require an image of each dimension rather than to by default allow one high-resolution image and optionally also allow for smaller images.

Cernelius commented 5 years ago

Yeah, @FrostionAAA, we are past that since some time. That's the reason I didn't even mention the possible need of having truly different images, as nobody is arguing against that. What has been discussed lately was the opportunity to have a single large image getting scaled down as basis, beside the additional options of having different images for the different places, without that being mandatory.

My answer was that I don't see a good point with that, unless its based on vector graphic.

What @DanVanAtta, then, said is that a very big image being an integer multiple of any wanted size can substantially work closely as well as a vector graphic image.

For example, in my case, I could just provide this raster image, that would practically scale down perfectly for any current uses (that is what I'm already doing, as my 128x128 image is actually a scaled down version of this one): conquered_big

That's substantially true, but it is sort of a work around about using raster images to do what is usually the field of vector graphic. It still doesn't seem the correct way to go, to me. On top of that, territory effects are maybe the most unpopular feature of a number of things all working a same way since ever; so really creating an exceptional resizing option for this niche case only would be awkward anyways (if we are talking about doing it for flags and whatever the like too, we are off topic).

Anyways, it is true that providing a very big image would go a way supporting future dimensional changes for the program in general (4k, 8k, whatever), but this should be really done through vector graphic.