Migrate `AvatarState.actionPoint` to use `ActionPointModule` #2502

sonohoshi · 2024-04-01T05:56:35Z

resolve #2496

🔼변경 이후 AvatarState.actionPoint 를 참조하는 곳들의 목록입니다. Serialize, 구버전 액션 이외엔 더 이상 사용하지 않도록 변경했습니다.

#2495 에서 변경된 ActionPoint 상태를 다른 액션에서도 반영하는 PR입니다.
변경되는 파일이 많아, 가능한 한 파일별로 변경이 생길때마다 커밋을 나눴습니다.
주로 ActionPointModule을 사용하게 변경한 커밋, 그리고 이를 단위 테스트에서 반영한 커밋으로 구분하였습니다.

별개로 AvatarState 생성자에서 GameConfigState를 받지 않게 제거하는 부분은 별개의 PR로 작업하는게 좋다고 판단되어 포함하지 않았습니다.

이하는 액션 변경 커밋들의 설명입니다.

82c37f5 에서 ItemEnhancement의 무의미한 actionPoint 참조를 제거
ea5897f 에서 ChargeActionPoint가 ActionPointModule을 사용하도록 반영
10a0651 에서 CombinationEquipment가 필요한 경우 ActionPointModule을 사용하도록 반영
62c6b62 에서 CombinationConsumable가 필요한 경우 ActionPointModule을 사용하도록 반영
d7b6f51 에서 EventConsumableItemCrafts가 필요한 경우 ActionPointModule을 사용하도록 반영
a50cf1a 에서 Grinding이 ActionPointModule을 사용하도록 반영
55822a3 에서 HackAndSlash가 ActionPointModule을 사용하도록 반영
3ae2140 에서 HAS Sweep이 ActionPointModule을 사용하도록 반영
3b170dd 에서 AvatarState.UseAp() 메소드를 InventoryExtensions로 옮겨서 ActionPointModule을 사용하도록 반영
6a5c4f3 에서 AvatarState.UseAp()를 쓰던 부분을 모두 InventoryExtensions.UseActionPoint() 를 쓰도록 반영
0a1f649 에서 SetActionPoint()를 통해 avatarState 대신 별개의 주소에 ActionPoint를 저장하도록 변경

더 이상 AvatarState에 있을 필요가 없어 역할을 옮겼습니다.

액션 하드포킹 테스트를 위해 쓰이고 있어서 동작을 이전과 같이 다시 수정했습니다

Validator 코드를 안쓴지 한참됐는데 usage를 갖고있어서 헷갈립니다.

pull-request-quantifier-deprecated · 2024-04-01T06:46:36Z

This PR has 951 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Large
Size       : +261 -690
Percentile : 98.37%

Total files changed: 38

Change summary by file extension:
.cs : +261 -690

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

ipdae · 2024-04-01T12:27:47Z

Lib9c/Helper/InventoryExtensions.cs

@@ -25,5 +30,39 @@ public static int GetEquippedFullCostumeOrArmorId(this Inventory inventory)

            return GameConfig.DefaultAvatarArmorId;
        }
+
+        public static IWorld UseActionPoint(


실제 사용은 인벤토리보다는 액션안에서 IWorld를 대상으로 하는 경우가 더 많을것같은데, ActionPointModule로 옮기고 인벤토리를 받게하는건 어떨까요? 아니면 state의 변경은 별도로 처리하고 이 메서드는 인벤토리에서 ap포션을 사용하는것까지만 책임지는게 더 좋을것같습니다. (추후에 최적화를 더 진행하게 된다면 인벤토리만 변경한후 IWorld에 Set하는 경우도 있을것 같아서)

인벤토리쪽에서 AP포션을 사용하는거까지만 책임지는게 나을거같아서 그 방향으로 변경해보겠습니다.

ipdae · 2024-04-01T12:32:25Z

Lib9c/Action/ChargeActionPoint.cs

-            }
-
-            if (avatarState.actionPoint == gameConfigState.ActionPointMax)
+            var actionPoint = states.GetActionPoint(avatarAddress);


diff에는 안잡히는데 이 액션은 UseActionPoint 를 사용하지 않나요?

네. 기존의 UseAp()는 ***Product 같은 아이템 판매/구매 관련 액션에서만 쓰고 있었습니다.

ipdae · 2024-04-01T12:39:53Z

Lib9c/Action/CreateAvatar.cs

@@ -238,7 +238,8 @@ public override IWorld Execute(IActionContext context)
            Log.Debug("{AddressesHex}CreateAvatar Total Executed Time: {Elapsed}", addressesHex, ended - started);
            return states
                .SetAgentState(signer, agentState)
-                .SetAvatarState(avatarAddress, avatarState);
+                .SetAvatarState(avatarAddress, avatarState)
+                .SetActionPoint(avatarAddress, DailyReward.ActionPointMax);


(아마 DailyReward에서 동작오류는 없겠지만) 새로 생성되는 아바타는 명시적으로SetDailyRewardReceivedBlockIndex 로 설정을 해주는게 좋을것같습니다.

@ipdae 기존에는 AvatarState() 생성자쪽에서도 set을 안해서 CreateAvatar 액션 실행 이후에도 0L로 set되는 형태였는데, 명시적으로 SetDailyRewardReceivedBlockIndex(avatarAddress, 0L) 같은 호출을 해주는게 좋을까요?
아니면 아예 생성 시점의 인덱스를 set해주는게 좋겠다는 말씀이셨을까요?

SetDailyRewardReceivedBlockIndex(avatarAddress, 0L) 을 하는게 좋겠다는 얘기였습니다.

이해했습니다 반영하도록 하겠습니다

ipdae

현재 수정된 액션들은 모두 GetActionPoint를 통해 마이그레이션된 ActionPoint가 체인에 존재하는것을 전제로 작성되있는데, 실제 체인에서는 DailyReward액션을 실행하거나 신규생성된 아바타가 아니면 마이그레이션된 상태가 존재하지 않는 상태입니다. ActionPointModule.TryGetActionPoint처럼 예외처리를 통해 마이그레이션이 안된 상태면 기존 아바타의 actionPoint를 사용하고, 마지막에 반환할때 SetActionPoint를 호출해 패시브 마이그레이션이 진행되도록 고려해야합니다.

at CreateAvatar action

do not SetState, return calculated result.

if can not get action point, use avatarState.actionPoint. and use ActionPointModule.SetActionPoint().

pull-request-quantifier-deprecated · 2024-04-02T06:26:35Z

This PR has 979 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Large
Size       : +286 -693
Percentile : 99.3%

Total files changed: 39

Change summary by file extension:
.cs : +286 -693

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

sonohoshi · 2024-04-02T06:29:13Z

현재 수정된 액션들은 모두 GetActionPoint를 통해 마이그레이션된 ActionPoint가 체인에 존재하는것을 전제로 작성되있는데, 실제 체인에서는 DailyReward액션을 실행하거나 신규생성된 아바타가 아니면 마이그레이션된 상태가 존재하지 않는 상태입니다. ActionPointModule.TryGetActionPoint처럼 예외처리를 통해 마이그레이션이 안된 상태면 기존 아바타의 actionPoint를 사용하고, 마지막에 반환할때 SetActionPoint를 호출해 패시브 마이그레이션이 진행되도록 고려해야합니다.

@ipdae ActionPointModule.TryGetActionPoint() 를 만들어서 여기서 false가 나온 애들은 avatarState.actionPoint를 쓴 뒤 최종적으론 소모된 후의 값을 ActionPointModule.SetActionPoint() 할 수 있게 변경했습니다.
그 외엔 InventoryExtensions.UseActionPoint() 가 IWorld를 변경하지 않고, 아이템과 AP 사용후 남은 AP를 계산하여 반환하도록 수정했습니다. 리뷰 다시 부탁드립니다

sonohoshi added 21 commits March 29, 2024 17:02

remove unnecessary action point validating at ItemEnhancement

82c37f5

remove abnormal GetRequiredAp invoking

b7933df

use ActionPointModule at ChargeActionPoint

ea5897f

reflect ChargeActionPoint hardfork to unit test

884a9d4

use ActionPointModule at CombinationEquipment

10a0651

reflect CombinationEquipment hardfork to unit test

ff2465b

use ActionPointModule at CombinationConsumable

62c6b62

reflect CombinationConsumable hardfork to unit test

77aaa3c

use ActionPointModule at EventConsumableItemCrafts

d7b6f51

reflect EventConsumableItemCrafts hardfork to unit test

cba4854

use ActionPointModule at Grinding

a50cf1a

reflect Grinding hardfork to unit test

bb77215

use ActionPointModule at HackAndSlash

55822a3

reflect HackAndSlash hardfork to unit test

98642c6

use ActionPointModule at HackAndSlashSweep

3ae2140

reflect HackAndSlashSweep hardfork to unit test

e101c0a

move AvatarState.UseAp to InventoryExtensions.UseActionPoint

3b170dd

더 이상 AvatarState에 있을 필요가 없어 역할을 옮겼습니다.

use Inventory.UseActionPoint instead AvatarState.UseAp

6a5c4f3

remove unit test about AvatarState.UseAp()

0f1b5a0

add unit test about InventoryExtensions.UseActionPoint

491eaee

change GameConfigState.ActionPointMax to DailyReward.ActionPointMax

705eb2b

sonohoshi requested review from ipdae and a team April 1, 2024 05:56

sonohoshi self-assigned this Apr 1, 2024

pull-request-quantifier-deprecated bot added the Large label Apr 1, 2024

sonohoshi changed the title ~~Migrate AvatarState.actionPoint to use ActionPointModule~~ (wip) Migrate AvatarState.actionPoint to use ActionPointModule Apr 1, 2024

sonohoshi marked this pull request as draft April 1, 2024 06:00

sonohoshi added 3 commits April 1, 2024 15:34

remove obsoleted action ReRegisterProduct0

3b7b0c9

revoke RegisterProduct0

f377b88

액션 하드포킹 테스트를 위해 쓰이고 있어서 동작을 이전과 같이 다시 수정했습니다

fix broken unit tests about ActionPoint hard-forking

ec37aad

pull-request-quantifier-deprecated bot added Extra Large and removed Large labels Apr 1, 2024

sonohoshi linked an issue Apr 1, 2024 that may be closed by this pull request

Action 최적화: DailyReward 하드포크를 다른 액션에도 반영 #2496

Closed

sonohoshi marked this pull request as ready for review April 1, 2024 06:36

sonohoshi changed the title ~~(wip) Migrate AvatarState.actionPoint to use ActionPointModule~~ Migrate AvatarState.actionPoint to use ActionPointModule Apr 1, 2024

sonohoshi added 3 commits April 1, 2024 15:42

add SetActionPoint() to CreateAvatar

0a1f649

add Assert.Equal about ActionPointModule with CreateAvatar

1a1eccb

remove unnecessary code

9add03b

Validator 코드를 안쓴지 한참됐는데 usage를 갖고있어서 헷갈립니다.

planetarium deleted a comment from pull-request-quantifier-deprecated bot Apr 1, 2024

ipdae reviewed Apr 1, 2024

View reviewed changes

ipdae requested changes Apr 2, 2024

View reviewed changes

sonohoshi added 6 commits April 2, 2024 11:42

reflect on review about SetDailyRewardReceivedBlockIndex

031cdb5

at CreateAvatar action

add ActionPointModule.TryGetActionPoint()

92d1db5

fix InventoryExtensions.UseActionPoint()

b07fa6a

do not SetState, return calculated result.

fix InventoryExtensionsTest for changed interface

cfd092b

fix many actions for passive-state migration

129faf3

if can not get action point, use avatarState.actionPoint. and use ActionPointModule.SetActionPoint().

fix unit tests, use TryGetActionPoint

9f98111

sonohoshi requested a review from ipdae April 2, 2024 06:29

sonohoshi mentioned this pull request Apr 2, 2024

Action 최적화: DailyReward 하드포크를 클라이언트에 반영 planetarium/NineChronicles#4656

Closed

ipdae approved these changes Apr 2, 2024

View reviewed changes

sonohoshi merged commit 1dcb552 into improve/action-execute Apr 2, 2024
12 checks passed

sonohoshi deleted the migrate/for-action-point branch April 2, 2024 09:13

sonohoshi mentioned this pull request Apr 3, 2024

reflect Hardforking of DailyReward and ActionPoint planetarium/NineChronicles#4721

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `AvatarState.actionPoint` to use `ActionPointModule` #2502

Migrate `AvatarState.actionPoint` to use `ActionPointModule` #2502

sonohoshi commented Apr 1, 2024 •

edited

Loading

pull-request-quantifier-deprecated bot commented Apr 1, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

ipdae Apr 1, 2024

sonohoshi Apr 2, 2024

ipdae Apr 1, 2024

sonohoshi Apr 2, 2024

ipdae Apr 1, 2024

sonohoshi Apr 2, 2024

ipdae Apr 2, 2024

sonohoshi Apr 2, 2024

ipdae left a comment

pull-request-quantifier-deprecated bot commented Apr 2, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

sonohoshi commented Apr 2, 2024

Migrate AvatarState.actionPoint to use ActionPointModule #2502

Migrate AvatarState.actionPoint to use ActionPointModule #2502

Conversation

sonohoshi commented Apr 1, 2024 • edited Loading

pull-request-quantifier-deprecated bot commented Apr 1, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipdae left a comment

Choose a reason for hiding this comment

pull-request-quantifier-deprecated bot commented Apr 2, 2024

What can I do to optimize my changes

How to interpret the change counts in git diff output

sonohoshi commented Apr 2, 2024

Migrate `AvatarState.actionPoint` to use `ActionPointModule` #2502

Migrate `AvatarState.actionPoint` to use `ActionPointModule` #2502

sonohoshi commented Apr 1, 2024 •

edited

Loading