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

(bleed) AutoRenderSize returns int2.Zero for WithSpriteBody with any required condition #14449

Closed
reaperrr opened this Issue Nov 28, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@reaperrr
Contributor

reaperrr commented Nov 28, 2017

The bug was introduced (or at least triggered) by the split of Render and Selection bounds.
On bleed this breaks the RA V2 launcher (making it invisible and unselectable), so this is obviously a playtest blocker.

As long as one of the actors' sprite bodies doesn't have any required condition, it works fine, but even with correct condition setup (like for the V2), it breaks when all bodies have some kind of required condition, even if one of them is/should be currently "active".

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 28, 2017

Member

The traitinfo has an EnabledByDefault field that we could use as a workaround.

Member

pchote commented Nov 28, 2017

The traitinfo has an EnabledByDefault field that we could use as a workaround.

@MunWolf

This comment has been minimized.

Show comment
Hide comment
@MunWolf

MunWolf Nov 29, 2017

Contributor

AutoRenderSize checks if (b.Animation.Animation.CurrentSequence) in the where, what if none of the bodies have a CurrentSequence because they have never been enabled? (Shot in the dark here)

Contributor

MunWolf commented Nov 29, 2017

AutoRenderSize checks if (b.Animation.Animation.CurrentSequence) in the where, what if none of the bodies have a CurrentSequence because they have never been enabled? (Shot in the dark here)

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 30, 2017

Member

@MunWolf They have CurrentSequences, but they are not enabled, yes.

@pchote
We're talking about RenderSprites checking Animations, so WithSpriteBody's EnabledByDefault doesn't help here.
Regardless, here are two possible solutions I came up with:

Option 1. IF (and that is a very big "IF" that I don't believe in) it's safe to cache the size of a random (whichever is first) sequence, which was the old/current behaviour and use that forever (pretty sure "deploying" actors can easily invalidate that, but let's leave this for future PRs), then we can easily not care about which is the first visible sequence in order to get its size and just get the size of the first (visible or not).
The code in question is:

// Required by WithSpriteBody and WithInfantryBody
public int2 AutoRenderSize(Actor self)
{
return anims.Where(b => b.IsVisible
&& b.Animation.Animation.CurrentSequence != null)
.Select(a => (a.Animation.Animation.Image.Size.XY * info.Scale).ToInt2())
.FirstOrDefault();
}

I think we can safely drop the IsVisible from that and still keep the old behavior. At some point we'll probably want to not cache the size and then we'll need it back.
Also if we go with this I want to point out from now that we want the end result to look more like this

		public int2 AutoRenderSize(Actor self)
		{
			var anim = anims.FirstOrDefault(b => b.Animation.Animation.CurrentSequence != null);
			return anim == null ? int2.Zero : (anim.Animation.Animation.Image.Size.XY * info.Scale).ToInt2();
		}

Option 2. Due to the nature of Contitional traits and the fact that the condition results are applied after the actor is created, in the Actor's constructor there is actually nothing enabled (which you implied with that EnabledByDefault), but that is when the bounds are calculated. So if we deffer the calculations for a later time, then the issue solves itself because by the first Tick of the actor's lifetime it already has an enabled animation that provides a valid size.

Going with Option 2. may even be better in the long run as I'm pretty sure we'd want to get rid of the caching of the bounds, unless I'm missing something that modders can do to make it work when switching to a different "model" under some condition (deploying into something bigger comes to mind).

Member

penev92 commented Nov 30, 2017

@MunWolf They have CurrentSequences, but they are not enabled, yes.

@pchote
We're talking about RenderSprites checking Animations, so WithSpriteBody's EnabledByDefault doesn't help here.
Regardless, here are two possible solutions I came up with:

Option 1. IF (and that is a very big "IF" that I don't believe in) it's safe to cache the size of a random (whichever is first) sequence, which was the old/current behaviour and use that forever (pretty sure "deploying" actors can easily invalidate that, but let's leave this for future PRs), then we can easily not care about which is the first visible sequence in order to get its size and just get the size of the first (visible or not).
The code in question is:

// Required by WithSpriteBody and WithInfantryBody
public int2 AutoRenderSize(Actor self)
{
return anims.Where(b => b.IsVisible
&& b.Animation.Animation.CurrentSequence != null)
.Select(a => (a.Animation.Animation.Image.Size.XY * info.Scale).ToInt2())
.FirstOrDefault();
}

I think we can safely drop the IsVisible from that and still keep the old behavior. At some point we'll probably want to not cache the size and then we'll need it back.
Also if we go with this I want to point out from now that we want the end result to look more like this

		public int2 AutoRenderSize(Actor self)
		{
			var anim = anims.FirstOrDefault(b => b.Animation.Animation.CurrentSequence != null);
			return anim == null ? int2.Zero : (anim.Animation.Animation.Image.Size.XY * info.Scale).ToInt2();
		}

Option 2. Due to the nature of Contitional traits and the fact that the condition results are applied after the actor is created, in the Actor's constructor there is actually nothing enabled (which you implied with that EnabledByDefault), but that is when the bounds are calculated. So if we deffer the calculations for a later time, then the issue solves itself because by the first Tick of the actor's lifetime it already has an enabled animation that provides a valid size.

Going with Option 2. may even be better in the long run as I'm pretty sure we'd want to get rid of the caching of the bounds, unless I'm missing something that modders can do to make it work when switching to a different "model" under some condition (deploying into something bigger comes to mind).

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 30, 2017

Member

Deferring the calculation until after the Created calls have been run should be the best option here.

Member

pchote commented Nov 30, 2017

Deferring the calculation until after the Created calls have been run should be the best option here.

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