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

BLIT image function fails in Add-on Manager #2058

Open
Arcanister opened this Issue Sep 30, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@Arcanister
Contributor

Arcanister commented Sep 30, 2017

I have been experimenting with add-on publishing facility and get this message:

error config: Failed to apply a modification to an image:
Image: attacks/thorns.png.
Modifications: ~BLIT(icons/icon-addon-publish.png).
Error: ~BLIT(): offset and width '64' larger than destination image's width '60' no blitting performed.

As far, as I understand, this happens because of addon_list.cpp:151.
Add-on icon is supposed to be displayed with green + on top when it is publishable, my icon thorns.png was 60x60, while icon-addon-publish.png is 64x64.

I see at least two solutions to this problem:

  1. Do all this:
  • teach BLIT to recognize negative coordinates as coming from low-right corner
  • crop publish icon to green cross only
  • use something like ~BLIT(small-icon-publish.png,-16,-16).
  1. Add some other image path function in front of ~BLIT() in GUI. It could be either of:
  • Scale add-on icon to 64x64 and then BLIT() with cross
  • BLIT add-on icon to empty icon of desired size, such as 64x64 and apply BLIT(icon-publish.png) after that.

@Arcanister Arcanister changed the title from BLIT image function fails to BLIT image function fails in Add-on Manager Sep 30, 2017

@doofus-01

This comment has been minimized.

Show comment
Hide comment
@doofus-01

doofus-01 Sep 30, 2017

Member

Hi @Arcanister ,
I don't see this as an issue, really.

Another solution would be to add some other image path function in front of ~BLIT(), which expands add-on icon to desired size, such as 64x64 and apply BLIT after that.

You could just blit both images onto misc/blank-hex.png, and scale as needed.

It is intuitive that you can't fit a larger image into a smaller image. Scaling the larger image first, or not using the smaller image as a base, would fix it. I'd rather have the error than have the function switch to some other behavior when I don't use it correctly.

Member

doofus-01 commented Sep 30, 2017

Hi @Arcanister ,
I don't see this as an issue, really.

Another solution would be to add some other image path function in front of ~BLIT(), which expands add-on icon to desired size, such as 64x64 and apply BLIT after that.

You could just blit both images onto misc/blank-hex.png, and scale as needed.

It is intuitive that you can't fit a larger image into a smaller image. Scaling the larger image first, or not using the smaller image as a base, would fix it. I'd rather have the error than have the function switch to some other behavior when I don't use it correctly.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Oct 1, 2017

Member

I don't think it actually applies to this case, but allowing negative coordinates in BLIT does sound like a nice idea.

Member

CelticMinstrel commented Oct 1, 2017

I don't think it actually applies to this case, but allowing negative coordinates in BLIT does sound like a nice idea.

@Arcanister

This comment has been minimized.

Show comment
Hide comment
@Arcanister

Arcanister Oct 14, 2017

Contributor

@doofus-01 The issue is not in my add-on (which is really a simple test case), the problem is in Wesnoth main code which tries to BLIT() a larger image (+ icon) onto smaller image (user-choosen icon).

Contributor

Arcanister commented Oct 14, 2017

@doofus-01 The issue is not in my add-on (which is really a simple test case), the problem is in Wesnoth main code which tries to BLIT() a larger image (+ icon) onto smaller image (user-choosen icon).

@doofus-01

This comment has been minimized.

Show comment
Hide comment
@doofus-01

doofus-01 Oct 14, 2017

Member

@Arcanister - OK, then I'd still say BLIT() is behaving just fine, the issue is with the add-on manager GUI. Without testing, I won't speculate what exactly should be done, but changing the way BLIT() behaves surely isn't the answer.

Member

doofus-01 commented Oct 14, 2017

@Arcanister - OK, then I'd still say BLIT() is behaving just fine, the issue is with the add-on manager GUI. Without testing, I won't speculate what exactly should be done, but changing the way BLIT() behaves surely isn't the answer.

@Arcanister

This comment has been minimized.

Show comment
Hide comment
@Arcanister

Arcanister Oct 14, 2017

Contributor

Sure, it's not an answer by itself. I have changed initial message to clarify it, but I did mean all this from beginning.

Contributor

Arcanister commented Oct 14, 2017

Sure, it's not an answer by itself. I have changed initial message to clarify it, but I did mean all this from beginning.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Oct 14, 2017

Member

I dunno, it does seem to me that automatically clipping to the destination image's bounds would have made sense for BLIT though.

Member

CelticMinstrel commented Oct 14, 2017

I dunno, it does seem to me that automatically clipping to the destination image's bounds would have made sense for BLIT though.

@doofus-01

This comment has been minimized.

Show comment
Hide comment
@doofus-01

doofus-01 Oct 14, 2017

Member

@Arcanister - I'd still say there is nothing wrong with BLIT(). Your second solution seems reasonable, but I don't know why it should be 64x64px. 72px might be better, though even that might be problematic for people who want to use bigger images. Making BLIT( ) auto-detect the largest image might be simple, I have no idea, but it seems unnecessary; at the end of the day, you chose an image that didn't work, you got an error, and now you can easily fix it so that it does work. So, I'd still not be convinced anything is wrong here.

@CelticMinstrel - It might be nice in some circumstances, but I'd still rather have the error, since it meant I wasn't paying attention to the coordinates on the base image or the size of the applied image. For an image that is built up by macros, this could be an issue, silently leading to messed up images I might not see for a while.

Member

doofus-01 commented Oct 14, 2017

@Arcanister - I'd still say there is nothing wrong with BLIT(). Your second solution seems reasonable, but I don't know why it should be 64x64px. 72px might be better, though even that might be problematic for people who want to use bigger images. Making BLIT( ) auto-detect the largest image might be simple, I have no idea, but it seems unnecessary; at the end of the day, you chose an image that didn't work, you got an error, and now you can easily fix it so that it does work. So, I'd still not be convinced anything is wrong here.

@CelticMinstrel - It might be nice in some circumstances, but I'd still rather have the error, since it meant I wasn't paying attention to the coordinates on the base image or the size of the applied image. For an image that is built up by macros, this could be an issue, silently leading to messed up images I might not see for a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment