Skip to content
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

feat: add utility functions to set sprite order #318

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

LordMidas
Copy link
Member

@LordMidas LordMidas commented Oct 5, 2023

To be looked at after we finish port to Modern Hooks.

@LordMidas LordMidas requested review from TaroEld and Enduriel October 5, 2023 19:17
@LordMidas LordMidas added this to the 1.3.0 milestone Oct 5, 2023
Copy link
Member

@Enduriel Enduriel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to print a warning when there is a queue cycle

@LordMidas LordMidas requested a review from Enduriel October 20, 2023 03:54
@LordMidas
Copy link
Member Author

LordMidas commented Oct 20, 2023

I think that the fact that our MSU.Sprite.addSprite function adds all the sprites that are found in InFrontOf and Behind of a sprite when that sprite is being added is problematic. Because this means it may add those sprites in an order which isn't meant for those sprites. We should only deal with ordering with respect to ONE other sprite.

So, the function should in fact be:

function setOrder( _script, _sprite, _behind )

This way whenever the _behind sprite is being added, we add _sprite immediately before that.

Alternatively we allow the _behind in this function to be an array, and whenever any sprite in _behind is being added, we add _sprite before that.

This ensures that we don't end up adding sprites to an entity in an order that wasn't meant for those sprites.

@Enduriel
Copy link
Member

@LordMidas I actually agree, I think that simpler approach has all the utility of the more complex one without more strange edge cases

@TaroEld
Copy link
Member

TaroEld commented Oct 22, 2023

Do we have a use case for this?

@LordMidas
Copy link
Member Author

Do we have a use case for this?

Yeah e.g. in Reforged we add cape sprites to some Ancient Dead leaders. That cape has to appear above the armor but behind the helmet, so it needs to be added to the entity behind the entity's helmet sprite. The only way to do that right now would be to hook the addSprite function in a switcheroo way inside the onInit function of that entity. But what if we want to add this new rf_cape sprite to a dozen entities, now you have to switcheroo the addSprite function in a dozen entities in an ugly way as below:

function onInit()
{
	local addSprite = this.addSprite;
	this.addSprite = function( _sprite )
	{
		if (_sprite == "armor")
		{
			local ret = addSprite(_sprite);
			this.addSprite("rf_cape");
			return ret;
		}
		return addSprite(_sprite);
	}
    this.skeleton.onInit();
    this.addSprite = addSprite;
}

WIth this new method you set the order of that sprite to be behind behind helmet so whenever helmet is being added it adds this sprite before that. The way Enduriel is suggesting is that you would still end up doing it individually for all the dozen entities, but at least it won't look like above. Instead you can do it centrally in a place somewhere.

I have so far found myself to be more open to the switcheroo method above, so probably we don't need this new spriteOrder functionality. I think we should postpone it to 1.4.0 at least for now.

@Enduriel
Copy link
Member

Enduriel commented Dec 1, 2023

I've had to do this in layered items as well, it does have use cases though they are admittedly uncommon. I'm okay with postponing this too

@Enduriel Enduriel modified the milestones: 1.3.0, 1.4.0 Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants