Skip to content

Conversation

@LordMidas
Copy link
Member

This contains two deprecation commits. The feature of this PR is in the main feat commit, so you may wanna look into the diff of that commit only to get an overview of how it works.

I would like to deprecate the scheduled changes system of skills because, while it is an interesting idea, I think there are plenty of ways to work around it if you design/implement skills in a smart way. Personally I have never used it, and I believe it is a bloat to support further.

This is a massive improvement over the current Affordability Preview system we have in MSU which I implemented in #88. That system is convoluted to work with from the user's perspective and is also limited in certain aspects. The new system is far more intuitive and in fact covers some edge cases which the old system failed in.

Old System (the one this PR deprecates):

  • Basically requires you to use modifyPreviewField and modifyPreviewProperty functions from inside a skill's onAffordablePreview function. The parameters of those functions are convoluted to work with.
  • The system calculates the overall change by first "undoing" the change from the skill in its onUpdate, onAfterUpdate, and executeScheduledChanges functions and then applies the new change.
  • A major limitation of this system is that the changes are applied at the end, so during the affordability preview function you don't have access to the values of other skills which would be in the regular update order by the time this skill's function comes up.

New System:

  • We add two new functions onUpdatePreview and onAfterUpdatePreview which by default call their regular counterparts. But you can overwrite these functions for custom functionality during previewing.
  • This makes the system very straightforward.
  • On the back end the system achieves this by strategically placing "preview" type updates and then regular updates of the skill_container in between getting the affordability status of skills and their cost string. This is probably less "efficient" than the old system but honestly this is better and I wish I had done it this way the first time around.
  • This also solves the major issue above and in fact gives you FULL control during those preview functions to do whatever you want.

Example:
Let's say I have a skill which after at least 2 tiles reduces the AP cost of all attacks by 1. But this effect expires when you use any skill.

Old System usage:

function onAfterUpdate( _properties )
{
	if (this.getContainer().getActor().getTile().getDistanceTo(this.m.StartingTile) >= 2)
	{
		foreach (skill in this.getContainer().getAllSkillsOfType(::Const.SkillType.Active))
		{
			if (skill.isAttack() && skill.m.ActionPointCost > 1) // Notice we only want to decrease the AP cost if it is 2 or more
				skill.m.ActionPointCost -= 1;
		}
	}
}

function onAffordablePreview( _skill, _movementTile )
{
	// The character is previewing the usage of a skill, so we want to remove the AP cost reduction
	if (_skill != null)
	{
		foreach (skill in this.getContainer().getAllSkillsOfType(::Const.SkillType.Active))
		{			
			this.modifyPreviewField(skill, "ActionPointCost", 0, false);
		}
	}
	// The character is previewing a movement, so we want to apply the AP cost reduction if applicable
	else if (_movementTile != null)
	{
		// If we have already traveled at least 2 tiles this turn, we don't want to reduce the AP cost further
		if (this.getContainer().getActor().getTile().getDistanceTo(this.m.StartingTile) >= 2)
			return;

		if (_movementTile.getDistanceTo(this.getContainer().getActor().getTile()) >= 2)
		{
			foreach (skill in this.getContainer().getAllSkillsOfType(::Const.SkillType.Active))
			{	
				// Notice there is no way for us to first check here if the skill's AP cost 		
				// is actually 2 or more at this point in the update cycle, because this onAffordablePreview function
				// doesn't work like that
			if (skill.isAttack())
				this.modifyPreviewField(skill, "ActionPointCost", -1 , false);
			}
		}
	}
}

New System:

function onAfterUpdate( _properties )
{
	if (this.getContainer().getActor().getTile().getDistanceTo(this.m.StartingTile) >= 2)
	{
		foreach (skill in this.getContainer().getAllSkillsOfType(::Const.SkillType.Active))
		{
			if (skill.isAttack() && skill.m.ActionPointCost > 1) // Notice we only want to decrease the AP cost if it is 2 or more
				skill.m.ActionPointCost -= 1;
		}
	}
}

function onAfterUpdatePreview( _properties, _previewedSkill, _previewedMovement )
{
	// The character is previewing the usage of a skill
	if (_previewedSkill != null)
	{
		// Do nothing i.e. apply no bonus from this perk
	}
	// The character is previewing a movement, so we want to apply the AP cost reduction if applicable
	else if (_previewedMovement != null)
	{
		// If we have already traveled at least 2 tiles this turn, we don't want to reduce the AP cost further
		if (this.getContainer().getActor().getTile().getDistanceTo(this.m.StartingTile) >= 2)
			return;

		if (_previewedMovement.End.getDistanceTo(this.getContainer().getActor().getTile()) >= 2)
		{
			foreach (skill in this.getContainer().getAllSkillsOfType(::Const.SkillType.Active))
			{	
				// Because this function runs in the same order as onAfterUpdate, we can actually check for the
				// AP cost of the skill at this point (in addition to the "preview" changes from other skills) which is fantastic!
				if (skill.isAttack() && skill.m.ActionPointCost > 1)
					skill.m.ActionPointCost -= 1;
			}
		}
	}
}

@LordMidas LordMidas added this to the 1.4.0 milestone Apr 14, 2024
@LordMidas LordMidas requested review from Enduriel and TaroEld April 14, 2024 06:18
@LordMidas LordMidas force-pushed the feat-affordability-preview-improved branch 2 times, most recently from 2b19bc6 to a309d67 Compare April 15, 2024 03:10
@LordMidas
Copy link
Member Author

LordMidas commented Apr 19, 2024

One thing that I need Taro/Enduriel to improve in this system is that if the affordability preview would make a skill that is currently unaffordable affordable again, then we need to convey this information somehow. Currently, that skill's icon stays black&white because the vanilla affordability preview system only considers the case of currently affordable skills potentially becoming unaffordable and hence puts a "prohibited" icon on them. It does not expect currently unaffordable skills to become affordable. An idea for this case could be to put a green check mark on such skills to counter the red prohibited sign.

Relevant functions for achieving this are:

// in `turnsequencebar_module.js`:
TacticalScreenTurnSequenceBarModule.prototype.addSkillToList
TacticalScreenTurnSequenceBarModule.prototype.updateEntitySkillsPreview

// in `turn_sequence_bar.nut:`
setActiveEntityCostsPreview
resetActiveEntityCostsPreview

Instead of only via hard-coded functions like onUpdatePreview etc.
@LordMidas
Copy link
Member Author

Closing in favor of #354

@LordMidas LordMidas closed this May 10, 2024
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.

2 participants