Conversation
github-actions
bot
requested review from
bluemoon983,
leesiuuuu,
suti1110 and
wkdrudgnsdla
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a weapon synthesis system, including the EquipmentManager for synthesis logic, the Slot class for UI management, and updates to the SO_Weapon data structure. The review feedback highlights critical IndexOutOfRangeException risks when handling maximum-level weapons in both the UI refresh and synthesis methods. Additionally, a bug was identified in the synthesis condition that prevents processing when the exact required material count is met, and a recommendation was made to simplify UI calculation logic to avoid potential floating-point inaccuracies and improve readability.
| weaponBackgroundImage[i].color = weaponLevelColor[weaponLevelValue + i]; | ||
| weaponSlotImage[i].sprite = weaponArray[weaponTypeValue] | ||
| .weapons[weaponLevelValue + i] | ||
| .weaponIcon; |
There was a problem hiding this comment.
RefreshSynthesis 함수에서 현재 선택된 무기가 최고 등급(예: S 등급)일 경우, i = 1 루프 실행 시 weaponLevelValue + i가 배열 범위를 초과하여 IndexOutOfRangeException이 발생합니다. 최고 등급일 때는 합성이 불가능하므로 함수 시작 부분에서 등급을 체크하여 조기 반환(Early Return)하거나 UI 처리를 예외적으로 수행해야 합니다.
| { | ||
| weaponArray[weaponTypeValue].weapons[weaponLevelValue].weaponCount -= | ||
| (uint)synthesisCount * 5; | ||
| GetItem(weaponTypeValue, weaponLevelValue + 1, (uint)synthesisCount); |
There was a problem hiding this comment.
무기가 이미 최고 등급인 상태에서 Synthesis가 호출되면 weaponLevelValue + 1 인덱스 접근 시 에러가 발생합니다. 합성이 가능한 최대 등급인지 확인하는 방어 코드가 필요합니다.
| .weaponIcon; | ||
| string hexColor = "#" + ColorUtility.ToHtmlStringRGB(synthesisColor[i]); | ||
| weaponCountText[i].text = | ||
| $"{weaponArray[weaponTypeValue].weapons[weaponLevelValue + i].weaponCount}(<color={hexColor}>{synthesisCount * 5 * (i == 0 ? -1 : (1 / 5f))}</color>)"; |
There was a problem hiding this comment.
합성 결과 수치를 계산하는 로직(synthesisCount * 5 * (i == 0 ? -1 : (1 / 5f)))이 복잡하고 가독성이 떨어집니다. 또한 부동 소수점 연산(1 / 5f)으로 인해 UI에 소수점이 표시될 가능성이 있습니다. 정수 연산을 사용하여 명확하게 분리하는 것을 권장합니다.
|
|
||
| public void Synthesis() | ||
| { | ||
| if (synthesisCount * 5 < weaponArray[weaponTypeValue].weapons[weaponLevelValue].weaponCount) |
There was a problem hiding this comment.
합성에 필요한 무기 개수가 보유량과 정확히 일치할 때 합성이 진행되지 않는 버그가 있습니다. < 연산자를 <=로 수정해야 사용자가 보유한 모든 무기를 소모하여 합성할 수 있습니다.
if (synthesisCount * 5 <= weaponArray[weaponTypeValue].weapons[weaponLevelValue].weaponCount)
There was a problem hiding this comment.
WeaponLevel enum에서 C가 D보다 높게 쓰여있는 이유가 있을까요?
| @@ -24,13 +23,15 @@ public enum WeaponType | |||
| public class SO_Weapon : ScriptableObject | |||
| { | |||
There was a problem hiding this comment.
SO_Weapon클래스에서 WeaponId라는 속성을 추가로 넣는 것을 권장 드립니다
There was a problem hiding this comment.
WeaponID 부분과 같은 것들은 장비 탭을 만들 때 넣을 예정입니다
|
|
||
| public class EquipmentManager : MonoBehaviour | ||
| { | ||
| public static EquipmentManager Instance { get; private set; } |
There was a problem hiding this comment.
Instance프로퍼티에는 더 철저한 예외 처리가 필요합니다. Awake문에서 초기화를 한다고 해도 먼저 실행된 다른 Awake문이 있을 수도 있기 때문입니다
| public Image[] weaponBackgroundImage; | ||
| public Image[] weaponSlotImage; | ||
| public Text[] weaponCountText; | ||
| public Text synthesisCountText; |
There was a problem hiding this comment.
윗부분은 카멜 케이스인데 여기 부분만 파스칼 케이스로 쓴 이유가 있을까요?
There was a problem hiding this comment.
평소 명명규칙을 생각하지 않고 변수를 생성해서 생긴 문제 같습니다 명명규칙을 지켜서 작성하도록 하겠습니다
| private Sprite WeaponIcon; | ||
| public Sprite weaponIcon => WeaponIcon; |
There was a problem hiding this comment.
일반적으로 private 속성은 카멜 케이스, public 속성은 파스칼 케이스를 사용하는게 맞습니다.
3 participants
No description provided.