search

mt-mods / signs_lib

Other
4 stars 12 forks source link

Fix out-of-bounds in 'combine' texture modifier #25

Closed Niklp09 closed 4 months ago

Niklp09 commented 6 months ago

Fixes https://github.com/mt-mods/signs_lib/issues/24 TenPlus1 asked me to create this PR since github doesn't let him.

Tested, and works. But I'm not sure if this is the best approach :thinking: .

tenplus1 commented 6 months ago

@Niklp09 - Thanks for the assist :) I find that it generates signs quicker this way although anyone using their own custom font sets will need to use the convert.sh I've included to negate the image files so they work with the new setup.

wsor4035 commented 6 months ago

But I'm not sure if this is the best approach

agree on this, just skim'd the pr tho

tenplus1 commented 5 months ago

@Niklp09 + @wsor4035 - Does something worry you guys about the fix ?

appgurueu commented 5 months ago

FWIW, you could "negate" the image files using the ^[invert texture modifier to avoid messing with the media files.

I have to say though: The current media files seem like an ugly hack. The actual glyph is transparent, whereas the background is solid? Hence the approach of this PR - to invert alpha - makes sense. If maintaining backwards compat with "custom fonts" - without running a script - is desired, you could use ^[invert.

IMO, it would make the most sense to have white, opaque glyphs on transparent background. Then you could also use ^[multiply instead of ^[colorize, which would enable having a bit of shading for glyphs (rather than just plain solid color).

tenplus1 commented 5 months ago

@appgurueu - I did try to [invert media but it never worked properly on transparent areas and the unicode textures ended up with artifacts, so in the end editing the files had to be done by hand or script.

tenplus1 commented 5 months ago

If it worries anyone how the changes affect signs_lib, check out Xanadu server, we've been using it since the commit and it's working well.

appgurueu commented 5 months ago

I wouldn't mind the texture changes much. However, please at least run something like pngcrush - the new textures are much larger than they need to be. (You could also consider converting the textures to TGA to shave off some more bytes, or packing them up in a tilesheet.)

tenplus1 commented 5 months ago

@appgurueu - Run optipng on every texture, please find attached the final mod in .zip form. signs_lib_NEW.zip

Niklp09 commented 5 months ago

Commited to the branch, sorry for the delay.

appgurueu commented 5 months ago

Anything stopping this?

OgelGames commented 4 months ago

After some trial and error (the code is terrible to read), I figured out a simpler way to fix the issue: #27

OgelGames commented 4 months ago

I just tested this and it doesn't fix the issue, there are still out-of-bounds textures when there is text overflow.

tenplus1 commented 4 months ago

On further inspection the signs_libcolor(pxsize)_n.png files are required for the sign to display properly, but I have removed the colours not in use. I also applied the size changes from the OgelGames pull request into this one which works a little better, so here is the latest mod update attached :) signs_lib.zip

OgelGames commented 4 months ago

Merged #27, closing this.

tenplus1 commented 4 months ago

@OgelGames - Could you re-open this as a working alternative.

S-S-X commented 4 months ago

For now restored branch if someone is still working with it. Related issue was reopened so I guess meta discussion could go there.

OgelGames commented 4 months ago

Could you re-open this as a working alternative.

As I noted previously, this PR doesn't work either. I think a lot more is needed to fix it properly.

wsor4035 commented 4 months ago

cross posting

frankly replacing colorized textures with ^[colorize is a good idea as to the implementation, didnt exactly dig to deep into it, so perhaps it had some flaw, but the concept is good

appgurueu commented 4 months ago

If there was a good specification (and tests?) such that we could ensure low risk of breaking compatibility, I would be inclined to suggest a rewrite of this part of the logic: Generating text strings via [combine is relatively simple.

Is there a good specification though?

tenplus1 commented 4 months ago

Could you re-open this as a working alternative.

As I noted previously, this PR doesn't work either. I think a lot more is needed to fix it properly.

How so ? I've been using this on Xanadu and signs work fine.

OgelGames commented 4 months ago

How so ? I've been using this on Xanadu and signs work fine.

It doesn't fix the out-of-bounds errors when the text overflows past the edge of the sign.