factoriolib / flib

A set of high-quality, commonly-used utilities for creating Factorio mods.
https://mods.factorio.com/mod/flib
MIT License
61 stars 15 forks source link

Default scale in create_icons is incorrect #64

Closed kirazy closed 4 months ago

kirazy commented 4 months ago

https://github.com/factoriolib/flib/blob/86cf56ac9bb6f4a7b6ffbba7dcd3b8ac8ed2dfe4/data-util.lua#L74

This should not be 1, this should be 32 / icon_size for an item, 256 / icon_size for a technology.

This will cause sprite icons to be much larger than expected in a few places in the GUI, such as blueprints and alt-overlays.

raiguard commented 4 months ago

This is one of Optera's functions.

@0ptera Will changing this break your usecases?

0ptera commented 4 months ago

Not sure what it will break. When calling that function I have no way of knowing if that icon belongs to a tech or item.

When called I assume sprites have correct size so 64/icon_size for techs and 32/icon_size for anything else would equal 1 anyway.

kirazy commented 4 months ago

The default values are specified in the documentation. It is not 64/icon_size for techs, it is specifically called out as 256 / icon_size. Nor would 32/icon_size always equal 1: high-res icons are conventionally 64 pixels, which would be scale = 0.5.

There is no way to know whether you need to calculate the default scale for an icon or a technology without having that as a parameter. You cannot assume one or the other. And you cannot assume scale is a constant. If you don't know what it is, leave it nil. Otherwise you end up with this:

image

Because you set the scale to 1 in cases where it is unknown:

image

0ptera commented 4 months ago

I just tested it without defaulting to 1. It also makes LEPP generated icons play nice with your Bob reskin.

On that note, defaulting to icon_size 32 should still be fine? https://github.com/factoriolib/flib/blob/86cf56ac9bb6f4a7b6ffbba7dcd3b8ac8ed2dfe4/data-util.lua#L71

kirazy commented 4 months ago

I would suggest treating it as an error.

It's a required field on the IconData type, mod authors should provide it, because it corresponds to the actual size of an image and if there is a mismatch Factorio (may? always? I can't remember which off the top of my head) throw an exception anyways.

icon_size has to exist: it has to be on the prototype root (prototype.icon_size), or on the first each layer of prototype.icons e.g. prototype.icons[1].icon_size. If neither of those two exist, it's an invalid format and Factorio itself will throw an exception, so you may as well do the same.

0ptera commented 4 months ago

@raiguard I'll leave it to you to close the issue, since I have no idea how you handle bugfixes and releases.

raiguard commented 4 months ago

I close the issues as things are pushed, so I'll close this now. Thanks!