Skip to content
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.

Sign up for GitHub

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

Fix duplicate collection id #2508

Merged
merged 5 commits into from
Apr 4, 2024
Merged

Fix duplicate collection id #2508

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8dda29c
Refactor collection state modifiers
ipdae Apr 3, 2024
307023c
Fix duplicate activated collection ids
ipdae Apr 3, 2024
3fc752b
Use SortedSet instead of list
ipdae Apr 4, 2024
34ef59f
Fix test
ipdae Apr 4, 2024
8497a80
Merge branch 'development' into bugfix/issue-2501
ipdae Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .Lib9c.Tests/Model/State/CollectionStateTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ public void Bencoded()
{
1,
2,
1,
2,
},
};

Expand All @@ -27,7 +29,7 @@ public void Bencoded()
Assert.Equal(expected, serialized);

var deserialized = new CollectionState(serialized);
Assert.Equal(state.Ids, deserialized.Ids);
Assert.Equal(new List<int> { 1, 2, }, deserialized.Ids);
sonohoshi marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
6 changes: 1 addition & 5 deletions Lib9c/Action/BattleArena.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,11 +397,7 @@ public override IWorld Execute(IActionContext context)
foreach (var (address, state) in collectionStates)
#pragma warning restore LAA1002
{
var modifier = modifiers[address];
foreach (var collectionId in state.Ids)
{
modifier.AddRange(collectionSheet[collectionId].StatModifiers);
}
modifiers[address] = state.GetModifiers(collectionSheet);
}
}
for (var i = 0; i < ticket; i++)
Expand Down
5 changes: 1 addition & 4 deletions Lib9c/Action/EventDungeonBattle.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,10 +341,7 @@ is Bencodex.Types.List serializedEventDungeonInfoList
if (collectionExist)
{
var collectionSheet = sheets.GetSheet<CollectionSheet>();
foreach (var collectionId in collectionState.Ids)
{
collectionModifiers.AddRange(collectionSheet[collectionId].StatModifiers);
}
collectionModifiers = collectionState.GetModifiers(collectionSheet);
}

var deBuffLimitSheet = sheets.GetSheet<DeBuffLimitSheet>();
Expand Down
5 changes: 1 addition & 4 deletions Lib9c/Action/HackAndSlash.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,10 +467,7 @@ public IWorld Execute(
if (collectionExist)
{
var collectionSheet = sheets.GetSheet<CollectionSheet>();
foreach (var collectionId in collectionState.Ids)
{
collectionModifiers.AddRange(collectionSheet[collectionId].StatModifiers);
}
collectionModifiers = collectionState.GetModifiers(collectionSheet);
}

var deBuffLimitSheet = sheets.GetSheet<DeBuffLimitSheet>();
Expand Down
5 changes: 1 addition & 4 deletions Lib9c/Action/HackAndSlashSweep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,10 +256,7 @@ public override IWorld Execute(IActionContext context)
if (collectionExist)
{
var collectionSheet = sheets.GetSheet<CollectionSheet>();
foreach (var collectionId in collectionState.Ids)
{
collectionModifiers.AddRange(collectionSheet[collectionId].StatModifiers);
}
collectionModifiers = collectionState.GetModifiers(collectionSheet);
}

var costumeStatSheet = sheets.GetSheet<CostumeStatSheet>();
Expand Down
5 changes: 1 addition & 4 deletions Lib9c/Action/Raid.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,10 +217,7 @@ public override IWorld Execute(IActionContext context)
if (collectionExist)
{
var collectionSheet = sheets.GetSheet<CollectionSheet>();
foreach (var collectionId in collectionState.Ids)
{
collectionModifiers.AddRange(collectionSheet[collectionId].StatModifiers);
}
collectionModifiers = collectionState.GetModifiers(collectionSheet);
}
// Simulate.
var random = context.GetRandom();
Expand Down
18 changes: 17 additions & 1 deletion Lib9c/Model/State/CollectionState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
using System.Collections.Generic;
using System.Linq;
using Bencodex;
using Bencodex.Types;
using Nekoyume.Model.Stat;
using Nekoyume.TableData;

namespace Nekoyume.Model.State
{
Expand All @@ -22,12 +25,25 @@ public CollectionState(List serialized)
{
Ids.Add((Integer)value);
}

Ids = Ids.Distinct().ToList();
Copy link
Member

Choose a reason for hiding this comment

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

Ids를 Set과 같은 형태로 관리해도 되지 않을까요?

}

public CollectionState(IValue bencoded) : this((List)bencoded)
{
}

public IValue Bencoded => List.Empty.Add(new List(Ids));
public IValue Bencoded => List.Empty.Add(new List(Ids.Distinct()));
Copy link
Member

@sonohoshi sonohoshi Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

이거 Distinct 한 뒤에 List 내의 순서 같은게 달라서 상태 불일치가 생길 가능성은 없을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

직렬화된 상태가 순서에 영향을 안받는것으로 알고 있습니다. 정렬이 필요할까요? @planetarium/libplanet

Copy link
Member

Choose a reason for hiding this comment

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

아 그럼 이 내용은 엔진팀에서 확인&답변 부탁드립니다🙇‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Distinct 자체가 정렬을 담보하지는 않지만, List<T>.Distinct 자체가 원래 있던 순서를 유지하는 동작을 하고 있으므로 상관은 없을 겁니다.

다만 @sonohoshi 님이 말씀하신 대로 그렇게 좋은 프랙티스는 아니라고 생각하고, Active Migration 이후에 코드를 지우는 일은 필요 해 보이긴 합니다.


public List<StatModifier> GetModifiers(CollectionSheet collectionSheet)
{
var collectionModifiers = new List<StatModifier>();
foreach (var collectionId in Ids)
{
collectionModifiers.AddRange(collectionSheet[collectionId].StatModifiers);
}

return collectionModifiers;
}
}
}
Loading