Skip to content

Conversation

@WeylonSantana
Copy link
Contributor

@WeylonSantana WeylonSantana commented Mar 21, 2025

resolves #2686 | #1767

This feature aims to add custom states to resources based on their life.
works like in videos, and also supports animated resources

The only detail to pay attention to is that it will be considered a "dead state" when the maxhp of the state is 0%
if there is no state that fits the min-max hp percentages, nothing will be rendered
The current resources that already exist in the old model are migrated correctly (as far as testing) and already serve as a model for everyone who already has resources to understand how the system works.

resource rendering can be based on the resource's maximum lifetime or the resource object's maximum lifetime, which can provide a more dynamic mapping, but that is not the intent of this feature.

1742488880_Intersect_Editor.mp4
1742494850_Intersect_Client.mp4
1742521716_Intersect_Client.mp4

Copy link
Member

@pandinocoder pandinocoder left a comment

Choose a reason for hiding this comment

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

  1. MySQL migration is needed
  2. States should be sorted by their min/max health

public partial class ResourceStateDescriptor
{
public string Graphic { get; set; } = null;
public string Graphic { get; set; } = default;
Copy link
Member

Choose a reason for hiding this comment

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

TextureName

@@ -0,0 +1,8 @@
namespace Intersect.Framework.Core.GameObjects.Resources;

public enum ResourceGraphicType
Copy link
Member

Choose a reason for hiding this comment

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

ResourceTextureSource


public enum ResourceGraphicType
{
Graphic,
Copy link
Member

Choose a reason for hiding this comment

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

Resource

public string Graphic { get; set; } = null;
public string Graphic { get; set; } = default;

public ResourceGraphicType GraphicType { get; set; } = ResourceGraphicType.Graphic;
Copy link
Member

Choose a reason for hiding this comment

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

TextureType

public bool RenderBelowEntities { get; set; }

public bool GraphicFromTileset { get; set; }
public Guid AnimationId { get; set; } = Guid.Empty;
Copy link
Member

Choose a reason for hiding this comment

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

= Guid.Empty; isn't necessary

public static LocalizedString HealthStateNameError = @"Health Graphic State Name must be unique and greater than 0 characters.";

public static LocalizedString initialgraphic = @"Initial Graphic:";
public static LocalizedString HealthStateNameExists = @"Health Graphic State Name already exists.";
Copy link
Member

Choose a reason for hiding this comment

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

This isn't needed with the Guid change

public static LocalizedString initialgraphic = @"Initial Graphic:";
public static LocalizedString HealthStateNameExists = @"Health Graphic State Name already exists.";

public static LocalizedString hpregen = @"HP (%);";
Copy link
Member

Choose a reason for hiding this comment

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

image
This isn't needed, there's a label both above and to the right of the textbox, we only need the one to the right (but you can move it if you want, and we can reword it)

Comment on lines 21 to 43
SET HealthGraphics = json_object(
'Exhauted', json_object(
'Graphic', Exhausted_Graphic,
'GraphicType', Exhausted_GraphicFromTileset,
'RenderBelowEntities', Exhausted_RenderBelowEntities,
'Height', Exhausted_Height,
'Width', Exhausted_Width,
'X', Exhausted_X,
'Y', Exhausted_Y,
'MinHp', 0,
'MaxHp', 0
),
'Initial', json_object(
'Graphic', Initial_Graphic,
'GraphicType', Initial_GraphicFromTileset,
'RenderBelowEntities', Initial_RenderBelowEntities,
'Height', Initial_Height,
'Width', Initial_Width,
'X', Initial_X,
'Y', Initial_Y,
'MinHp', 100,
'MaxHp', 100
)
Copy link
Member

Choose a reason for hiding this comment

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

Will need to be updated to reflect name changes I mentioned

Copy link
Member

Choose a reason for hiding this comment

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

This will need to be regenerated after the renames

migrationBuilder.Sql(@"
UPDATE Resources
SET HealthGraphics = json_object(
'Exhauted', json_object(
Copy link
Member

Choose a reason for hiding this comment

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

Exhausted with an s

Comment on lines 25 to 39
'{exhaustedId}', json_object(
'Id', '{exhaustedId}',
'Name', 'Exhausted',
'Texture', Exhausted_Graphic,
'TextureType', Exhausted_GraphicFromTileset,
'RenderBelowEntities', Exhausted_RenderBelowEntities,
'Height', Exhausted_Height,
'Width', Exhausted_Width,
'X', Exhausted_X,
'Y', Exhausted_Y,
'MinimumHealth', 0,
'MaximumHealth', 0
),
'{initalId}', json_object(
'Id', '{initalId}',
Copy link
Member

Choose a reason for hiding this comment

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

All of the exhausted and initial states would have the same ID across resources.

While I think this won't cause an immediate issue, if we ever have to separate it into another table it will cause an issue (and we'd have to remember that this issue exists).

On Discord we discussed using invalid placeholder UUIDs for initial/exhausted and then re-assigning invalid IDs in InitDatabase().

Name = "New Resource";
Initial = new ResourceStateDescriptor();
Exhausted = new ResourceStateDescriptor();
StatesGraphics = [];
Copy link
Member

Choose a reason for hiding this comment

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

Do an inline = []; on States, don't use a constructor initializer

Name = "New Resource";
Initial = new ResourceStateDescriptor();
Exhausted = new ResourceStateDescriptor();
StatesGraphics = [];
Copy link
Member

Choose a reason for hiding this comment

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

Do an inline = []; on States, don't use a constructor initializer


public ResourceStateDescriptor Exhausted { get; set; }
[NotMapped, JsonIgnore]
public Dictionary<Guid, ResourceStateDescriptor> StatesGraphics { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

The = []; goes here


public ResourceStateDescriptor Exhausted { get; set; }
[NotMapped, JsonIgnore]
public Dictionary<Guid, ResourceStateDescriptor> StatesGraphics { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Also should be called States

[NotMapped, JsonIgnore]
public Dictionary<Guid, ResourceStateDescriptor> StatesGraphics { get; set; }

[Column("StatesGraphics")]
Copy link
Member

Choose a reason for hiding this comment

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

nameof(States)

set => StatesGraphics = JsonConvert.DeserializeObject<Dictionary<Guid, ResourceStateDescriptor>>(value);
}

[Column("Animation")]
Copy link
Member

Choose a reason for hiding this comment

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

nameof(Animation)

if (Texture == default)
{
Texture = Globals.ContentManager.GetTexture(Framework.Content.TextureType.Resource, _sprite);
Texture = Graphics.Renderer.WhitePixel;
Copy link
Member

Choose a reason for hiding this comment

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

Why are we setting it to a white pixel when we were not doing this before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when is a animation, Texture is null, if texture is null no hp bar is rendered because current code use player texture to render the hpbar, so assign a white pixel if any case above fail (not graphic on range, or animation), make sure to render hp bar when texture not exists when player hits

return;
}

if (!AnimationDescriptor.TryGet(_currentState.AnimationId, out var animationDescriptor))
Copy link
Member

Choose a reason for hiding this comment

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

The animation descriptor should be cached when the resource descriptor is changed (_descriptor)

Comment on lines 230 to 252
var currentState = graphicStates.FirstOrDefault(
s => currentHealthPercentage >= s.Value.MinimumHealth && currentHealthPercentage <= s.Value.MaximumHealth
);
Copy link
Member

Choose a reason for hiding this comment

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

Multiple problems here

  1. You are still not using .Values before the FirstOrDefault()
  2. You can check _currentState before doing this -- if _currentState is null or if the current health is outside of the range of _currentState then you go and change the state, you don't need to change the state if the current state is still valid.

Comment on lines 225 to 227
var maxHealth = Descriptor.UseExplicitMaxHealthForResourceStates
? Descriptor.MaxHp
: MaxVital[(int)Enums.Vital.Health];
Copy link
Member

Choose a reason for hiding this comment

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

This can be cached when the descriptor is set, cache it in _maximumHealthForStates

public void UpdateCurrentState()
{
if (descriptor == null)
if (Descriptor == default)
Copy link
Member

Choose a reason for hiding this comment

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

This risks a null due to race condition below, the descriptor was passed into this method for a reason, the other valid alteration is if (_descriptor is not { } descriptor), but you can't switch to Descriptor

/// <inheritdoc />
public override bool CanBeAttacked => !IsDead;

public ResourceStateDescriptor? CurrentGraphicState => _currentState;
Copy link
Member

Choose a reason for hiding this comment

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

CurrentState


private readonly int _tileWidth = Options.Instance.Map.TileWidth;
private readonly int _tileHeight = Options.Instance.Map.TileHeight;
private readonly int _tapHeight = Options.Instance.Map.MapHeight;
Copy link
Member

Choose a reason for hiding this comment

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

_mapHeight not _tapHeight

Comment on lines 422 to 423
_renderBoundsSrc.Width = Texture.Width;
_renderBoundsSrc.Height = Texture.Height;
Copy link
Member

Choose a reason for hiding this comment

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

you need null protection for Texture

Comment on lines 870 to 872
if (entity.Type == EntityType.Resource)
{
((Resource)entity).UpdateCurrentState();
Copy link
Member

Choose a reason for hiding this comment

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

this should be entity is Resource resourceEntity and then use resourceEntity instead of casting

Copy link
Member

@pandinocoder pandinocoder left a comment

Choose a reason for hiding this comment

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

This isn't a complete review, after addressing the comments I added you should check the part of the code I didn't review this time around and see if any of the things I mentioned elsewhere apply to code I hadn't commented on yet

I'll re-review (and hopefully do a more thorough review) after the current comments are addressed

Comment on lines 1478 to 1479
float xpos = x * Options.Instance.Map.TileWidth + xoffset + Options.Instance.Map.TileWidth / 2;
float ypos = y * Options.Instance.Map.TileHeight + yoffset + Options.Instance.Map.TileHeight / 2;
Copy link
Member

Choose a reason for hiding this comment

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

2f

var animationInstance = tmpMap.GetAttributeAnimation(tmpMap.Attributes[x, y], animation.Id);

//Update if the animation isn't right!
if (animationInstance == null || animationInstance.Descriptor != animation)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't animationInstance?.Descriptor != animation work?

if (animationInstance == null || animationInstance.Descriptor != animation)
{
tmpMap.SetAttributeAnimation(
tmpMap.Attributes[x, y], new Animation(animation, true)
Copy link
Member

Choose a reason for hiding this comment

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

newline between the arguments of SetAttributeAnimation

Comment on lines 19 to 20
var initalId = Guid.NewGuid();
var exhaustedId = Guid.NewGuid();
Copy link
Member

Choose a reason for hiding this comment

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

This still isn't matching what we discussed


private static void ValidateResourceStates()
{
var checkedIds = new List<Guid>();
Copy link
Member

Choose a reason for hiding this comment

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

This isn't what we had discussed

Comment on lines 162 to 165
item.States = item.States
.OrderBy(s => s.Value.MinimumHealth)
.ThenBy(s => s.Value.MaximumHealth)
.ToDictionary(p => p.Key, p => p.Value);
Copy link
Member

Choose a reason for hiding this comment

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

It should already be sorted before a user clicks save, it needs to get sorted when values are changed and the displayed list order needs to match the sorted order


//General Editting Variables
bool mTMouseDown;
private readonly List<ResourceDescriptor> _changed = [];
Copy link
Member

Choose a reason for hiding this comment

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

This should be a Dictionary<Guid, ResourceDescriptor>

if (_editorItem != null && lstGameObjects.Focused)
{
if (DarkMessageBox.ShowWarning(
Strings.ResourceEditor.deleteprompt, Strings.ResourceEditor.deletetitle, DarkDialogButton.YesNo,
Copy link
Member

Choose a reason for hiding this comment

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

1 parameter per line

Comment on lines 76 to 78
if (_editorItem != null && lstGameObjects.Focused)
{
if (DarkMessageBox.ShowWarning(
Copy link
Member

Choose a reason for hiding this comment

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

Invert the if statements to denest the logic

Comment on lines 125 to 128
toolStripItemCopy.Enabled = _editorItem != null && lstGameObjects.Focused;
toolStripItemPaste.Enabled = _editorItem != null && _copiedItem != null && lstGameObjects.Focused;
toolStripItemDelete.Enabled = _editorItem != null && lstGameObjects.Focused;
toolStripItemUndo.Enabled = _editorItem != null && lstGameObjects.Focused;
Copy link
Member

Choose a reason for hiding this comment

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

I see _editorItem != null && lstGameObjects.Focused repeated 4 times

public static LocalizedString StateNameError = @"State Name must be greater than 0 characters.";

public static LocalizedString initialgraphic = @"Initial Graphic:";
public static LocalizedString hpregen = @"HP Regen (%):";
Copy link
Member

Choose a reason for hiding this comment

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

HealthRegenPercent

Comment on lines 27 to 29
private readonly List<string> _knownFolders = [];
private readonly BindingList<NotifiableDrop> _dropList = [];
private readonly BindingList<ResourceStateDescriptor> _stateList = [];
Copy link
Member

Choose a reason for hiding this comment

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

Can you reorder this so all of the readonlys are together at the top with one empty line between them and the mutable fields?

Comment on lines 1419 to 1420
float xpos = x * Options.Instance.Map.TileWidth + xoffset;
float ypos = y * Options.Instance.Map.TileHeight + yoffset;
Copy link
Member

Choose a reason for hiding this comment

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

This "base" calculation is common between all 3 cases, please extract the common code

case ResourceTextureSource.Resource:
{
var texture = GameContentManager.GetTexture(
GameContentManager.TextureType.Resource, resourceGraphic.TextureName
Copy link
Member

Choose a reason for hiding this comment

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

1 arg per line

case ResourceTextureSource.Tileset:
{
var texture = GameContentManager.GetTexture(
GameContentManager.TextureType.Tileset, resourceGraphic.TextureName
Copy link
Member

Choose a reason for hiding this comment

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

1 arg per line

}

_currentState = currentState;
_sprite = _currentState?.TextureName ?? string.Empty;
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer we set _sprite to null if at all possible, rather than Empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then this would have to be left for another refactoring, since _sprite is defined as not null in entity, and this affects the Sprite property which is also not null, which would require adding several other checks in loadtextures, sprite set, and so on

Comment on lines 301 to 302
Descriptor = descriptor;
UpdateCurrentState();
Copy link
Member

Choose a reason for hiding this comment

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

Can we make the setter for Descriptor call UpdateCurrentState() instead of calling it separately?

(IsDead && !Descriptor.Exhausted.RenderBelowEntities) ||
(!IsDead && !Descriptor.Initial.RenderBelowEntities)
)
if (Descriptor == default || CurrentState is not { } graphicState || !graphicState.RenderBelowEntities)
Copy link
Member

Choose a reason for hiding this comment

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

We aren't using Descriptor at all in this if statement, I think it should be removed from the check here

Comment on lines 466 to 490
if (IsDead)
{
if (graphicState is { MaximumHealth: 0 } deadGraphic)
{
_renderBoundsSrc = new(
deadGraphic.X * _tileWidth,
deadGraphic.Y * _tileHeight,
(deadGraphic.Width + 1) * _tileWidth,
(deadGraphic.Height + 1) * _tileHeight
);
}
else
{
_renderBoundsSrc = new();
}
}
else if (graphicState is { MinimumHealth: > 0 } aliveGraphic)
{
_renderBoundsSrc = new(
aliveGraphic.X * _tileWidth,
aliveGraphic.Y * _tileHeight,
(aliveGraphic.Width + 1) * _tileWidth,
(aliveGraphic.Height + 1) * _tileHeight
);
}
Copy link
Member

Choose a reason for hiding this comment

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

The logic here is almost the same and I'd prefer it not get duplicated, and it seems suspicious that we don't have the "else" case for alive but we do for dead -- store deadGraphic/aliveGraphic in a local variable, and if the variable is null set the src to empty (use default instead of new()), and if it isn't use the local variable to calculate the positions


public string Name { get; set; }

public string TextureName { get; set; } = default;
Copy link
Member

Choose a reason for hiding this comment

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

string? must be nullable

Comment on lines 63 to 68
if (value == _descriptor)
{
return;
}

_descriptor = value;
Copy link
Member

Choose a reason for hiding this comment

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

This stuff needs to be before anything else in the setter, it shouldn't be after the new value is being used

Comment on lines 471 to 475
if (IsDead && graphicState is { MaximumHealth: 0 } deadGraphic)
{
if (graphicState is { MaximumHealth: 0 } deadGraphic)
{
_renderBoundsSrc = new(
deadGraphic.X * _tileWidth,
deadGraphic.Y * _tileHeight,
(deadGraphic.Width + 1) * _tileWidth,
(deadGraphic.Height + 1) * _tileHeight
);
}
else
{
_renderBoundsSrc = new();
}
selectedGraphic = deadGraphic;
}
else if (graphicState is { MinimumHealth: > 0 } aliveGraphic)
else if (!IsDead && graphicState is { MinimumHealth: > 0 } aliveGraphic)
Copy link
Member

Choose a reason for hiding this comment

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

IsDead shouldn't be getting checked twice.

(resourceGraphic.Width + 1) * Options.Instance.Map.TileWidth,
(resourceGraphic.Height + 1) * Options.Instance.Map.TileHeight, renderTarget
(resourceGraphic.Width + 1) * tileWidth,
(resourceGraphic.Height + 1) * Options.Instance.Map.TileHeight,
Copy link
Member

Choose a reason for hiding this comment

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

Options.Instance.Map.TileHeight

xpos,
ypos,
resourceGraphic.X * tileWidth,
resourceGraphic.Y * Options.Instance.Map.TileHeight,
Copy link
Member

Choose a reason for hiding this comment

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

Options.Instance.Map.TileHeight

float xpos = x * Options.Instance.Map.TileWidth + xoffset + Options.Instance.Map.TileWidth / 2f;
float ypos = y * Options.Instance.Map.TileHeight + yoffset + Options.Instance.Map.TileHeight / 2f;
xpos += tileWidth / 2f;
ypos += Options.Instance.Map.TileHeight / 2f;
Copy link
Member

Choose a reason for hiding this comment

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

Options.Instance.Map.TileHeight

}

float xpos = x * tileWidth + xoffset;
float ypos = y * Options.Instance.Map.TileHeight + yoffset;
Copy link
Member

Choose a reason for hiding this comment

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

Options.Instance.Map.TileHeight

var tileWidth = Options.Instance.Map.TileWidth;
var tileHeight = Options.Instance.Map.TileHeight;
var xoffset = CurrentView.Left + gridX * tileWidth * Options.Instance.Map.MapWidth;
var yoffset = CurrentView.Top + gridY * Options.Instance.Map.TileHeight * Options.Instance.Map.MapHeight;
Copy link
Member

Choose a reason for hiding this comment

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

Options.Instance.Map.TileHeight and Options.Instance.Map.MapHeight

var xoffset = CurrentView.Left + gridX * Options.Instance.Map.TileWidth * Options.Instance.Map.MapWidth;
var tileWidth = Options.Instance.Map.TileWidth;
var tileHeight = Options.Instance.Map.TileHeight;
var xoffset = CurrentView.Left + gridX * tileWidth * Options.Instance.Map.MapWidth;
Copy link
Member

Choose a reason for hiding this comment

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

Options.Instance.Map.MapWidth

}

var x1 = 0;
var x2 = Options.Instance.Map.MapWidth;
Copy link
Member

Choose a reason for hiding this comment

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

Options.Instance.Map.MapWidth should be cached and reused in the instances after this

Comment on lines 1527 to 1528
float xpos = x * tileWidth + xoffset + tileWidth / 2;
float ypos = y * Options.Instance.Map.TileHeight + yoffset + Options.Instance.Map.TileHeight / 2;
Copy link
Member

Choose a reason for hiding this comment

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

By the way both of these lines can be written as (base + 0.5f) * multiplier + offset

@pandinocoder pandinocoder merged commit da1ae0d into AscensionGameDev:main Mar 30, 2025
1 check passed
@pandinocoder pandinocoder deleted the resource-state branch March 30, 2025 18:40
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.

feature: Resources states

2 participants