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.

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

Feature/synthesize item #3001

Merged

Conversation

eugene-doobu
Copy link
Member

@eugene-doobu eugene-doobu self-assigned this Nov 14, 2024
@eugene-doobu eugene-doobu linked an issue Nov 14, 2024 that may be closed by this pull request
@eugene-doobu eugene-doobu added this to the v260.0.0 milestone Nov 14, 2024
@eugene-doobu eugene-doobu changed the base branch from development to feature/growth12 November 14, 2024 09:38
@ipdae
Copy link
Member

ipdae commented Nov 25, 2024

@eugene-doobu 이 PR은 언제 다시 열리는걸까요?

확인했습니다.


public int GradeId { get; private set; }
public int RequiredCount { get; private set; }
public float SucceedRate { get; private set; }
Copy link
Member

Choose a reason for hiding this comment

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

타입이 float이면 부동소수점 계산시 오류가 발생할 수 있을것 같습니다. decimal을 쓰거나 만분율을 쓰는게 좋을것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

76e3528 에서 적용해두었습니다

Copy link
Member Author

Choose a reason for hiding this comment

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

아래 pr에서 다시 int로 변경하였습니다.
a472a34

Copy link
Member

@ipdae ipdae left a comment

Choose a reason for hiding this comment

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

지금은 재료에 대한 검증 및 삭제 처리보다 합성을 먼저 처리하고 있는것 처럼 보입니다. 순서를 바꿔서 검증을 통해 조건이 안맞는 경우 우선 실패하도록 처리해야합니다.

var itemSubType = subTypeItem.Key;
var materialCount = subTypeItem.Value;

if (!synthesizeSheet.TryGetValue(gradeId, out var synthesizeRow))
Copy link
Member

Choose a reason for hiding this comment

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

이 gradeId는 루프안에서 변경되는 정보가 아닌것 같습니다. 윗라인으로 옮겨서 호출횟수를 줄일 수 있을것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

아래 커밋에서 수정했습니다
bf33ba9

}

var randomValue = GetRandomValueForItem(grade, synthesizeResultPool, synthesizeWeightSheet, random, out var itemWeights);
float cumulativeWeight = 0;
Copy link
Member

Choose a reason for hiding this comment

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

float이 아니라 decimal이나 만분율을 써야할 것 같습니다. 성능문제때문에 가급적이면 만분율을 추천합니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

아래 pr에서 변경했습니다
6a6a08e
6b18bf3

}

private float GetRandomValueForItem(Grade grade, HashSet<int> synthesizeResultPool, SynthesizeWeightSheet synthesizeWeightSheet,
Copy link
Member

Choose a reason for hiding this comment

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

이쪽도 반환타입이 float이면 안됩니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

아래 pr에서 변경했습니다
6a6a08e
6b18bf3

@@ -30,6 +30,7 @@ public class Synthesize : GameAction
private const string TypeIdentifier = "synthesize";

private const string MaterialsKey = "m";
private const string ChargeApKey = "c";
Copy link
Member

Choose a reason for hiding this comment

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

실제 ap충전 동작이 구현안되있는것 같습니다. (ap포션 사용처리)

Copy link
Member Author

Choose a reason for hiding this comment

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

아래 커밋에서 구현했습니다. 별도 pr로 뺄까 하다가 구현하기 쉬워보여서 같이 넣자 하고 삽 떠볼려고 필드만 먼저 추가해놨었습니당..
bf33ba9

@eugene-doobu
Copy link
Member Author

아이템 해제 순서 변경했습니다 @ipdae
4d11831

// Calculate success for each synthesis
for (var i = 0; i < synthesizeCount; i++)
{
var isSuccess = random.Next(SynthesizeSheet.SucceedRateMax) < synthesizeRow.SucceedRate;
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 Author

Choose a reason for hiding this comment

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

아래 pr에서 추가해두었습니다
3274722

@ipdae
Copy link
Member

ipdae commented Nov 27, 2024

코드 관련 summary 추가 부탁드립니다. CI가 깨지네요.

@eugene-doobu eugene-doobu merged commit 7f2929e into planetarium:feature/growth12 Nov 27, 2024
12 of 13 checks passed
@eugene-doobu eugene-doobu deleted the feature/synthesize-item branch November 27, 2024 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Synthesize Items
2 participants