Suggestion for {HIGHLIGHT_IMAGE} macro

[WhiteWolf]

Hello,

I have a small suggestion for the above mentioned macro. Currently, only background images can be protected with the BACKGROUND parameter, but background halos are lost no matter what.
I'd suggest to change the BACKGROUND paramater to a yes/no boolean that would set if the background items (both images and halos, any number) would be protected, or erased during the blinking. This is possible by giving a name to the item being blinked. Here is an example of a possible new macro definition, highlighted where something is changed:

Proposed new macro def:

Code: Select all

# in definition, removed BACKGROUND_IMAGE, added PROTECT_BACKGROUND parameter

#define HIGHLIGHT_IMAGE X Y IMAGE PROTECT_BACKGROUND
    # Scrolls to the given location, blinks the given image in and out a few
    # times, and then leaves the image in place. Use this to pinpoint an
    # important location to the player. If there's an existing [item] on the
    # location, use yes in PROTECT_BACKGROUND so that it is not erased.
    
    #-------------------------------------------- new
    {VARIABLE protect_background {PROTECT_BACKGROUND}}
    #----------------------------------------------
    
    [scroll_to]
        x,y={X},{Y}
        check_fogged=no
    [/scroll_to]

    [repeat]
        times=3
        [do]
            [item]
                x,y={X},{Y}
                halo={IMAGE}
                #------------------------------ new
                name=item_generated_by_HI_at_{X}_{Y}
                #--------------------------------
            [/item]

            [redraw][/redraw]

            [delay]
                time=300
            [/delay]
            
            #--------------------- removed
            [remove_item]
                x,y={X},{Y}
            [/remove_item]
            [item]
                x,y={X},{Y}
                image={BACKGROUND_VALUE}
            [/item]
            #----------------------
            
            #--------------------- instead, new
            [if]
                [variable]
                    name=protect_background
                    equals=yes
                [/variable]
                [then]
                    [remove_item]
                        name=item_generated_by_HI_at_{X}_{Y}
                    [/remove_item]
                [/then]
                [else]
                    [remove_item]
                        x,y={X},{Y}
                    [/remove_item]
                [/else]
            [/if]
            #--------------------
            

            [redraw][/redraw]

            [delay]
                time=300
            [/delay]
        [/do]
    [/repeat]

    [item]
        x,y={X},{Y}
        image={IMAGE}
    [/item]

    [redraw][/redraw]
    
    # new, clear the var:
    {CLEAR_VARIABLE protect_background}
#enddef

Great advantage is that it's backwards compatible with all {HIGHLIGHT_IMAGE x y image ()} calls, and behaves as it used to. Of course, if someone has {HIGHLIGHT_IMAGE x y image (specified_background)}, that has to be changed to {HIGHLIGHT_IMAGE x y image yes}

If it's worth the effort to extend backwards compatibility, then the definition

Code: Select all

#define HIGHLIGHT_IMAGE X Y IMAGE BACKGROUND_IMAGE
#arg PROTECT
no
#endarg

....
#enddef

could be used, where the new argument is optional for future use, and old code will still work. It has the advantage that if you want to protect all items on the location, you can use PROTECT=yes, and if you want to protect only one specific item, and erase all others, then you can specify that as BACKGROUND_IMAGE.

[josteph]

This is a bit hard to follow. If I understand correctly, your suggestion is to add a new version of {HIGHLIGHT_IMAGE} that, instead of taking {BACKGROUND_VALUE} that specifies a particular image to preserve, takes a yes/no parameter that means "preserve all images on that hex"? That sounds like a reasonable addition. I'm not sure if it's practical to make {HIGHLIGHT_IMAGE} support all of yes/no/()/filename for the fourth argument, though. If it is, great. If not, we can always add a new macro for the new functionality.

[WhiteWolf]

If I understand correctly, your suggestion is to add a new version of {HIGHLIGHT_IMAGE} that, instead of taking {BACKGROUND_VALUE} that specifies a particular image to preserve, takes a yes/no parameter that means "preserve all images on that hex"? That sounds like a reasonable addition.

Yes, that's exactly the proposition. Let's suppose you have an item with a halo animation, or multiple items on that same hex, and you want to highlight the gohere.png for the player there. The current macro will erase your background no matter what (because the current implementation can only protect items with "image", and only one item at that).
In the meantime I defined the suggested macro for my add-on, and what's in the original post doesn't work, I had to do it a bit differently:

New macro that works and does what was suggested:

Code: Select all

#define TRS_HIGHLIGHT_IMAGE X Y IMAGE PROTECT
    # Scrolls to the given location, blinks the given image in and out a few
    # times, and then leaves the image in place. Use this to pinpoint an
    # important location to the player. If there's an existing [item] on the
    # location, specify {PROTECT} as yes to protect it from being erased.
    [scroll_to]
        x,y={X},{Y}
        check_fogged=no
    [/scroll_to]

	{VARIABLE protect_bck {PROTECT}}
	[if]
		[variable]
			name=protect_bck
			equals=yes
		[/variable]
		[then]
			[store_items]
				x,y={X},{Y}
				variable=background
			[/store_items]
		[/then]
	[/if]

    [repeat]
        times=3
        [do]
            [item]
                x,y={X},{Y}
                halo={IMAGE}
				name=image_placed_by_macro
            [/item]

            [redraw][/redraw]

            [delay]
                time=300
            [/delay]

			[if]
				[variable]
					name=protect_bck
					equals=yes
				[/variable]
				[then]
       			    [remove_item]
                		x,y={X},{Y}
            		[/remove_item]

					{FOREACH background i}
						[if]
							[variable]
								name=background[$i].image
								contains=png
							[/variable]
							[then]
								[item]
									x,y=$background[$i].x,$background[$i].y
									image=$background[$i].image
								[/item]
							[/then]
							[else]
								[item]
									x,y=$background[$i].x,$background[$i].y
									halo=$background[$i].halo
								[/item]
							[/else]
						[/if]
					{NEXT i}
				[/then]
				[else]
       			    [remove_item]
                		x,y={X},{Y}
            		[/remove_item]
				[/else]
			[/if]


            [redraw][/redraw]

            [delay]
                time=300
            [/delay]
        [/do]
    [/repeat]

    [item]
        x,y={X},{Y}
        image={IMAGE}
    [/item]
	{CLEAR_VARIABLE protect_bck,background}
    [redraw][/redraw]
#enddef

So if this macro was mainline, that's be cool.
I think it could be merged with the existing macro, so that the last argument can take any of yes/no/()/filename. You could do:

Code: Select all

{VARIABLE check_on_this {LAST_ARGUMENT}}

And then filter on its value. Firstly, yes and no are easy to filter for, then you can identify that it's a filename with

Code: Select all

[variable]
    name=check_on_this
    contains="png"
[/variable]

And then anything else is regarded as (). Then in the macro, behave accordingly to this variable. So I think it can be done. However, indeed I'm not sure either which would be the better idea: to have a new macro, or to make the old one support the new utility.

[josteph]

I'm leaning toward having two separate macros. With one macro it'll be two easy to forget to handle all case when we change the macro. Could you please (fix the indentation and) open a pull request for discussing this further?

[WhiteWolf]

I'm leaning toward having two separate macros. With one macro it'll be two easy to forget to handle all case when we change the macro. Could you please (fix the indentation and) open a pull request for discussing this further?

So sorry for the very late answer I was completely absent for the past week.
Sorry for the indentation, I usually use tab(4), and when I copy mainline code and insert my stuff in it, usually this happens with the [code] on the forums. I'll fix that and open an issue

[Celtic_Minstrel]

I'd rather see the functionality added as a new WML tag rather than extending the macro. I've even implemented it already, so if someone wanted to copy that to mainline, I wouldn't mind.