Refactor camera and scene-related functionality into dedicated `Camer… #8
Conversation
Summary of ChangesHello @Mosch0512, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a substantial refactoring effort to improve the codebase's structure and maintainability. It systematically extracts various functionalities related to scene handling, camera control, and common utilities from a monolithic Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 is a significant and valuable refactoring effort, extracting scene and camera-related functionality into dedicated files. This greatly improves the project's structure and maintainability. My review focuses on ensuring correctness after this large code movement, identifying potential security vulnerabilities, and suggesting further improvements for code clarity and maintainability. I've found a few critical issues, such as an out-of-bounds array access and potential buffer overflows, that should be addressed before merging. I've also included several suggestions to reduce code duplication and improve readability.
| else if (gMapManager.WorldActive == WD_51HOME_6TH_CHAR | ||
| ) |
There was a problem hiding this comment.
There appears to be a syntax error here due to a dangling parenthesis on a new line. This will cause a compilation failure.
else if (gMapManager.WorldActive == WD_51HOME_6TH_CHAR)| { | ||
| for (int i = 0; i < num; ++i) | ||
| { | ||
| wcscpy(g_lpszMessageBoxCustom[i], GlobalText[index + i]); |
There was a problem hiding this comment.
The use of wcscpy is unsafe and can lead to a buffer overflow, especially since the destination buffer g_lpszMessageBoxCustom has a small fixed size (MAX_LENGTH_CMB is 38). This is a critical security vulnerability. Please use a safer function like wcscpy_s or wcsncpy.
| } | ||
|
|
||
| if (message == MESSAGE_USE_STATE2) | ||
| swprintf(g_lpszMessageBoxCustom[0], L"( %ls%ls )", Name, GlobalText[1901]); |
There was a problem hiding this comment.
Using swprintf without checking the buffer size (g_lpszMessageBoxCustom) is dangerous and can lead to a buffer overflow, which is a critical security vulnerability. The buffer size MAX_LENGTH_CMB is defined as 38, which is very small. You should use a safer alternative like swprintf_s or manually check the length of the resulting string before writing to the buffer.
Mosch0512
force-pushed
the
refactore-scenes
branch
2 times, most recently
from
05e8a2c to
db7238f
Compare
|
/gemini summary |
Summary of ChangesThis pull request undertakes a significant refactoring of the game's core scene management and utility functions. By extracting various functionalities from a single, large source file into multiple specialized files, the changes aim to enhance modularity, improve code maintainability, and establish a clearer separation of concerns. This architectural improvement will facilitate easier development, debugging, and future expansion of game features. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
|
Mosch0512
force-pushed
the
refactore-scenes
branch
from
26fd106 to
5166682
Compare
…aUtility` and `CharacterScene` files.
… and update constants for character handling across scenes.
Mosch0512
force-pushed
the
refactore-scenes
branch
from
5166682 to
865712b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the game's scene management and camera logic by extracting scene-specific and common utility functions from ZzzScene.cpp into new, dedicated files. The CameraUtility.cpp and CameraUtility.h files now encapsulate MoveMainCamera() and related camera variables, which is subsequently called by LoginScene.cpp, CharacterScene.cpp, and MainScene.cpp. A new SceneCommon.cpp and SceneCommon.h house general utility functions like abuse filters, UI helpers, audio settings, and rendering information, previously mixed within ZzzScene.cpp. The WebzenScene.cpp and WebzenScene.h are created for the initial splash screen. Finally, SceneManager.cpp and SceneManager.h are introduced to orchestrate the overall scene updates and rendering, dispatching calls to the appropriate scene-specific move and render functions. The _define.h file also adds MAX_CHARACTERS_PER_ACCOUNT as a constant. Review comments highlight several areas for improvement, including adding bounds checks for SelectedHero access, null pointer checks for the Hero object, clarifying intentional fall-throughs in switch statements, seeding the random number generator for camera movements, simplifying complex if-else structures, replacing magic numbers with named constants, and removing unused variables.
| { | ||
| g_pMainFrame->ResetSkillHotKey(); | ||
|
|
||
| g_ConsoleDebug->Write(MCD_NORMAL, L"Join the game with the following character: %ls", CharactersClient[SelectedHero].ID); |
There was a problem hiding this comment.
The SelectedHero index is used to access CharactersClient without a bounds check. If SelectedHero is out of the valid range (e.g., uninitialized or corrupted), this could lead to an out-of-bounds access and a crash. Add a check to ensure SelectedHero is within 0 and MAX_CHARACTERS_PER_ACCOUNT - 1 before accessing CharactersClient. This issue also appears on lines 62 and 69.
| g_ConsoleDebug->Write(MCD_NORMAL, L"Join the game with the following character: %ls", CharactersClient[SelectedHero].ID); | |
| if (SelectedHero >= 0 && SelectedHero < MAX_CHARACTERS_PER_ACCOUNT) | |
| { | |
| g_ConsoleDebug->Write(MCD_NORMAL, L"Join the game with the following character: %ls", CharactersClient[SelectedHero].ID); | |
| } | |
| else | |
| { | |
| // Handle error or log invalid SelectedHero | |
| g_ConsoleDebug->Write(MCD_ERROR, L"Invalid SelectedHero index: %d", SelectedHero); | |
| return; // Or other appropriate error handling | |
| } |
|
|
||
| vec3_t pos; | ||
|
|
||
| if (MoveMainCamera() == true) |
There was a problem hiding this comment.
The Hero pointer is dereferenced (Hero->Object.StartPosition) without checking if it is NULL. This could lead to a null pointer dereference and application crash if Hero is not properly initialized or becomes invalid. Add a null check before dereferencing Hero. This issue also appears on line 291.
if (Hero != nullptr)
{
VectorCopy(Hero->Object.StartPosition, pos);
}
else
{
// Handle error or log null Hero pointer
return false; // Or other appropriate error handling
}| case 5: | ||
| case 4: CameraViewFar = 3200.f; break; |
There was a problem hiding this comment.
The switch statement has case 5: immediately followed by case 4:. If case 5 is intended to have distinct logic, it should either include a break; or its logic should be placed before case 4. If it's an intentional fall-through, it should be explicitly commented for clarity to avoid potential logic errors.
case 3: CameraViewFar = 2950.f; break;
case 4:
case 5: // Fall-through from case 5 to case 4 is intentional
CameraViewFar = 3200.f; break;| CurrentCameraNumber = rand() % 4 + 1; | ||
| CurrentCameraWalkType = rand() % 2; |
There was a problem hiding this comment.
The rand() function is used to generate random camera numbers and walk types without explicitly seeding the random number generator (e.g., with srand(time(NULL))). This can lead to predictable or identical sequences of 'random' camera movements each time the application starts. Ensure srand() is called once at the application's initialization.
| if (gMapManager.InChaosCastle() == false || !Hero->Object.m_bActionStart) | ||
| { | ||
| // Skip height adjustment for Kanturu 3rd when action has started | ||
| if (gMapManager.WorldActive == WD_39KANTURU_3RD && Hero->Object.m_bActionStart) | ||
| { | ||
| // No-op: Hero maintains current Z position during action in Kanturu 3rd | ||
| } | ||
| else | ||
| if (gMapManager.WorldActive == -1 || Hero->Helper.Type != MODEL_HORN_OF_DINORANT || Hero->SafeZone) | ||
| { | ||
| Hero->Object.Position[2] = RequestTerrainHeight(Hero->Object.Position[0], Hero->Object.Position[1]); | ||
| } | ||
| else | ||
| { | ||
| if (gMapManager.WorldActive == WD_8TARKAN || gMapManager.WorldActive == WD_10HEAVEN) | ||
| Hero->Object.Position[2] = RequestTerrainHeight(Hero->Object.Position[0], Hero->Object.Position[1]) + 90.f; | ||
| else | ||
| Hero->Object.Position[2] = RequestTerrainHeight(Hero->Object.Position[0], Hero->Object.Position[1]) + 30.f; | ||
| } | ||
| } |
There was a problem hiding this comment.
The nested if-else structure in this block is quite complex and can be difficult to follow. Consider refactoring this logic to improve readability, perhaps by using early returns or a more flat structure.
|
|
||
| if (gMapManager.InBattleCastle() == true) | ||
| { | ||
| CameraPosition[2] = 255.f;//700 |
There was a problem hiding this comment.
The value 255.f appears to be a magic number. Consider defining this as a named constant to improve readability and maintainability. The commented //700 also suggests a previous value or a related constant.
CameraPosition[2] = BATTLE_CASTLE_CAMERA_HEIGHT; // Example constant name| { | ||
| CameraPosition[2] = g_fSpecialHeight = 1200.f + 1; | ||
| } | ||
| CameraPosition[2] += CameraDistance - 150.f;//700 |
There was a problem hiding this comment.
The value 150.f appears to be a magic number. Consider defining this as a named constant to improve readability and maintainability. The commented //700 also suggests a previous value or a related constant.
CameraPosition[2] += CameraDistance - CAMERA_DISTANCE_OFFSET; // Example constant name| { | ||
| CameraAngle[0] = -84.5f; | ||
| CameraAngle[1] = 0.0f; | ||
| CameraAngle[2] = -75.0f; | ||
| CameraPosition[0] = 9758.93f; | ||
| CameraPosition[1] = 18913.11f; | ||
| CameraPosition[2] = 675.5f; |
There was a problem hiding this comment.
Hardcoding specific camera position and angle values (-84.5f, 0.0f, -75.0f, 9758.93f, 18913.11f, 675.5f) for CHARACTER_SCENE makes it difficult to adjust these parameters without code changes. Consider defining these as constants or loading them from a configuration file for easier management.
| switch (g_shCameraLevel) | ||
| { | ||
| case 0: CameraDistanceTarget = 1000.f; CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; break; | ||
| case 1: CameraDistanceTarget = 1100.f; CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; break; | ||
| case 2: CameraDistanceTarget = 1200.f; CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; break; | ||
| case 3: CameraDistanceTarget = 1300.f; CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; break; | ||
| case 4: CameraDistanceTarget = 1400.f; CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; break; | ||
| case 5: CameraDistanceTarget = g_Direction.m_fCameraViewFar; CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; break; | ||
| } |
There was a problem hiding this comment.
The line CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; is repeated in multiple case statements. This can be refactored to avoid duplication, perhaps by placing it after the switch statement if CameraDistanceTarget is always set within the switch.
| switch (g_shCameraLevel) | |
| { | |
| case 0: CameraDistanceTarget = 1000.f; CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; break; | |
| case 1: CameraDistanceTarget = 1100.f; CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; break; | |
| case 2: CameraDistanceTarget = 1200.f; CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; break; | |
| case 3: CameraDistanceTarget = 1300.f; CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; break; | |
| case 4: CameraDistanceTarget = 1400.f; CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; break; | |
| case 5: CameraDistanceTarget = g_Direction.m_fCameraViewFar; CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; break; | |
| } | |
| case 0: CameraDistanceTarget = 1000.f; break; | |
| case 1: CameraDistanceTarget = 1100.f; break; | |
| case 2: CameraDistanceTarget = 1200.f; break; | |
| case 3: CameraDistanceTarget = 1300.f; break; | |
| case 4: CameraDistanceTarget = 1400.f; break; | |
| case 5: CameraDistanceTarget = g_Direction.m_fCameraViewFar; break; | |
| } | |
| CameraDistance += (CameraDistanceTarget - CameraDistance) / 3; |
| RenderTerrainAlphaBitmap(BITMAP_GM_AURORA, o->Position[0], o->Position[1], 1.2f, 1.2f, vLight, -WorldTime * 0.01f); | ||
| DisableAlphaBlend(); | ||
|
|
||
| float Rotation = (int)WorldTime % 3600 / (float)10.f; |
There was a problem hiding this comment.
The variable Rotation is declared and assigned a value but is never used. This is dead code and can be removed to improve code clarity and reduce potential for confusion.
…aUtility
andCharacterScene` files.