Skip to content

Conversation

@interfacew
Copy link
Contributor

No description provided.

Copy link
Collaborator

@Vessv Vessv left a comment

Choose a reason for hiding this comment

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

For now this is what i could find in a bit of time, there might be others things.

Copy link
Collaborator

@Vessv Vessv left a comment

Choose a reason for hiding this comment

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

Some other things i could find. As beofre, there are some suggestions, always check if the code makes sense lol(surely i didn't mess smt up).

Copy link
Collaborator

@Vessv Vessv left a comment

Choose a reason for hiding this comment

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

This are some other things i found.
Another thing is, i think you need to check all the modifiers you implemented and only add StatusType: when its a explicit buff or debuff. For example, i think the A4 modifier doesn't have that in datamine, so its prob better to remove it.

@interfacew
Copy link
Contributor Author

in the game, when dan unleash an enhanced attack, there is only 1 action.
but in this code, it used multi action(1 attack and 1-3 skill).

also in game, use the skill of dan won't trigger skill listener like yukong trace 103 to generate energy.

how can i fix this?

(sorry for my bad English)

@Vessv
Copy link
Collaborator

Vessv commented Sep 26, 2023

in the game, when dan unleash an enhanced attack, there is only 1 action. but in this code, it used multi action(1 attack and 1-3 skill).

also in game, use the skill of dan won't trigger skill listener like yukong trace 103 to generate energy.

how can i fix this?

(sorry for my bad English)

Hi, no worries, i am also a non native english speaker.
I also am not sure how 💀
But from what Kyle told me

from a sim perspective, we dont care about the internal distinction of multi actions like how it is implemented in game (at least not right now, though there is an argument to be made that we should). QQ is another case off the top of my head that has multiple skills technically. As long as the behavior is there, it doesn't materially matter what the action is since we do not have any "action keys"
The current implementation is definitely "incorrect" because of the whole "skill would count as an action" stuff, but that's okay because materially the implementation of DHIL should not change when that is fixed. It is okay to have the "incorrect" implementation merged it and then someone can follow up with the core changes necessary to support it
-Kyle

So don't worry about that for now, i think.

Copy link
Collaborator

@Vessv Vessv left a comment

Choose a reason for hiding this comment

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

This are the last changes i could find.
This is just to replicate how the game does it and so it doesn't buff other attacks like Tingyun additional(pursued) attack.

@Vessv Vessv mentioned this pull request Sep 27, 2023
1 task
Copy link
Collaborator

@Vessv Vessv left a comment

Choose a reason for hiding this comment

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

👍

@Vessv Vessv merged commit 1fad9a0 into simimpact:main Sep 28, 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