-
Notifications
You must be signed in to change notification settings - Fork 4
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
Affordability preview improved 2 #354
Open
LordMidas
wants to merge
13
commits into
development
Choose a base branch
from
feat-affordability-preview-improved-2
base: development
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of having it in every skill. This should be more memory efficient.
- This allows directly modifying fields/properties inside the onUpdate and onAfterUpdate functions for the preview case. - Is extensible automatically to all functions e.g. is usable within onUpdate, onAfterUpdate, isUsable etc.
This improves performance by hooking only the relevant skills which may still be using the older deprecated system instead of doing a full pseudo-update cycle on all skills. It leverages the fact that the newer system already does an update during preview.
This was referenced May 10, 2024
Otherwise the vanilla preview system completely breaks down (i.e. doesn't even show selected skill or preview fatigue or etc.) while a skill is being previewed. This happens because we do skill_container.update calls during the setActiveEntityCostsPreview in turn_sequence_bar and the update function calls setDirty which forces the actor's UI to update. This somehow causes some conflicts.
The entire point of the switcheroo on JSHandle is so that the original function is called before our skill container event (onAffordablePreview) so that the values set by the original function happen before our event.
Because it was getting reset during the skill_container.update call in setActiveEntityCostsPreview.
So that in the cost string of skill tooltips the color of the property (e.g. Action points or Fatigue) which will make a skill unaffordable will be properly colored red.
LordMidas
force-pushed
the
feat-affordability-preview-improved-2
branch
from
July 16, 2024 02:49
1a99b92
to
41d577a
Compare
Postponed as I am going to first implement, test and polish this system over at the Modular Vanilla mod. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Should be used instead of #346 because this implements it in a way where the affordability preview is inherently usable in all pre-existing functions instead of introducing new functions.
Contains a few perf and refactor changes. But primarily:
update
calls to skill_container while flipping a Boolean inside actor. This allows modders to check that an actor is previewing by accessing that Boolean from anywhere.The meat of this PR is the singular
feat
commit so you may wanna look at the diff of that commit to understand how the new system works.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):
modifyPreviewField
andmodifyPreviewProperty
functions from inside a skill'sonAffordablePreview
function. The parameters of those functions are convoluted to work with.onUpdate
,onAfterUpdate
, andexecuteScheduledChanges
functions and then applies the new change.New System:
onUpdate
,onAfterUpdate
and even any other function.skill_container
while flipping a Boolean inactor
to switch between previewing and regular updates. 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.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:
New System:
Help needed:
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: