-
Notifications
You must be signed in to change notification settings - Fork 0
Skill editor #11
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
Skill editor #11
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 delivers a significant new feature by integrating a full-fledged skill editor into the MuEditor. The editor provides a user-friendly interface for managing skill data, backed by a robust and modular data handling architecture. A key improvement is the adoption of a metadata-driven approach for UI rendering, which enhances flexibility for future data structure changes. Furthermore, the update expands the editor's internationalization capabilities with new language support and improved font rendering, making the tool more accessible to a global user base. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a comprehensive Skill Editor feature, including a new UI, data handlers for loading/saving/exporting skill data, and extensive internationalization support. The refactoring to use X-Macros and a common metadata helper for both Item and Skill editors is a significant improvement for maintainability. However, there are several issues to address. Multiple new translation files contain untranslated English strings. There are also critical bugs in swprintf calls with incorrect arguments, and a hardcoded font path that will cause issues on systems where Windows is not installed on C:.
Source Main 5.2/source/MuEditor/UI/ItemEditor/ItemEditorActions.cpp
Outdated
Show resolved
Hide resolved
Source Main 5.2/source/MuEditor/UI/ItemEditor/ItemEditorActions.cpp
Outdated
Show resolved
Hide resolved
Source Main 5.2/source/MuEditor/UI/SkillEditor/SkillEditorActions.cpp
Outdated
Show resolved
Hide resolved
647353a to
64bc941
Compare
0211224 to
40757ec
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new Skill Editor, a significant feature that includes a data-driven UI, new data handlers for skill information, and extensive internationalization support for many new languages. The implementation is well-structured, notably with the excellent use of X-Macros for defining data structures and a generic metadata helper for UI rendering, which greatly improves maintainability and extensibility. The dynamic loading of available languages and enhanced Unicode font support are also major improvements. My review focuses on a few areas for improvement: completing translations in all language files, addressing code duplication for a helper function, and replacing platform-specific code for directory scanning to ensure cross-platform compatibility.
| WIN32_FIND_DATAW findData; | ||
| const wchar_t* paths[] = {L"Translations\\*", L"bin\\Translations\\*"}; | ||
|
|
||
| for (const wchar_t* basePath : paths) | ||
| { | ||
| currentIndex = i; | ||
| break; | ||
| HANDLE hFind = FindFirstFileW(basePath, &findData); | ||
|
|
||
| if (hFind != INVALID_HANDLE_VALUE) | ||
| { | ||
| do | ||
| { | ||
| if ((findData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) && | ||
| wcscmp(findData.cFileName, L".") != 0 && | ||
| wcscmp(findData.cFileName, L"..") != 0) | ||
| { | ||
| // Convert wide string to narrow | ||
| char localeCode[32]; | ||
| WideCharToMultiByte(CP_UTF8, 0, findData.cFileName, -1, localeCode, sizeof(localeCode), NULL, NULL); | ||
| std::string locale = localeCode; | ||
|
|
||
| // Build path to editor.json | ||
| std::wstring editorJsonPath; | ||
| if (wcscmp(basePath, L"Translations\\*") == 0) | ||
| { | ||
| editorJsonPath = std::wstring(L"Translations\\") + findData.cFileName + L"\\editor.json"; | ||
| } | ||
| else | ||
| { | ||
| editorJsonPath = std::wstring(L"bin\\Translations\\") + findData.cFileName + L"\\editor.json"; | ||
| } | ||
|
|
||
| // Read language name from JSON file | ||
| std::string displayName = ReadLanguageName(editorJsonPath); | ||
|
|
||
| // Fallback to locale code if reading failed | ||
| if (displayName.empty()) | ||
| { | ||
| displayName = locale; | ||
| } | ||
|
|
||
| availableLanguages.push_back({locale, displayName}); | ||
| } | ||
| } while (FindNextFileW(hFind, &findData)); | ||
|
|
||
| FindClose(hFind); | ||
| } | ||
|
|
||
| // If we found languages, no need to try alternate path | ||
| if (!availableLanguages.empty()) | ||
| { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for auto-detecting available languages by scanning the Translations directory is implemented using Windows-specific APIs (WIN32_FIND_DATAW, FindFirstFileW, etc.). This will cause compilation errors or incorrect behavior on other platforms like macOS or Linux. Consider using a cross-platform library like C++17's <filesystem> or a third-party library to handle directory iteration.
Source Main 5.2/source/DataHandler/SkillData/SkillDataSaver.cpp
Outdated
Show resolved
Hide resolved
2c788a2 to
8721688
Compare
6ef9588 to
c7100da
Compare
91ad71b to
ecf6cc8
Compare
02386a3 to
562245e
Compare
…IBUTE - Create GameData/SkillData/SkillStructs.h with SKILL_ATTRIBUTE definition - Remove duplicate SKILL_ATTRIBUTE from _struct.h, replace with include - SkillStructs.h is now single source of truth for ALL builds (editor and release) - Keep SKILL_ATTRIBUTE_FILE in _struct.h for binary file I/O - Establishes pattern: all game data structures will be organized in GameData/ directory This modernizes the codebase structure and eliminates duplicate definitions.
- Create SkillFieldDefs.h with X-macro definitions for all SKILL_ATTRIBUTE fields - Define 20 simple fields (Level, Damage, Mana, Distance, etc.) - Define 10 array fields (RequireDutyClass, RequireClass) - Include 2 DWORD fields (Distance, SkillBrand) using DWord type from Commit 1 - Add type helper macros for C++ type conversion - Total: 30 editable fields + Name field (31 columns in skill editor table) This is the single source of truth for skill field metadata.
- Created GameData/Common/FieldMetadataHelper.h with shared EFieldType enum, generic FieldDescriptor<TStruct> template, and RenderFieldByDescriptor() function - Refactored GameData/ItemData/ItemFieldMetadata.h (51% code reduction, 112 → 55 lines) - Created GameData/SkillData/SkillFieldMetadata.h using shared infrastructure - Added FIELD_TYPE_DWord to GameData/ItemData/ItemFieldDefs.h - Updated 9 ItemEditor files to use explicit ItemFieldDescriptor type - Fixed friend declaration to match generic template signature with 8 parameters Eliminates ~80 lines of duplication, establishes single source of truth for field metadata patterns, and prepares for future editors without code duplication.
- Added 20 skill field translations to bin/Translations/en/metadata.json (field_Damage, field_Mana, field_AbilityGuage, field_Distance, etc.) - Added 3 RequireDutyClass array field translations - Added skill-specific buttons, messages, and labels to bin/Translations/en/editor.json (btn_save_skills, msg_save_skills_success, label_skill_editor_title, etc.) Note: Some fields (field_Level, field_Strength, field_Dexterity) already existed from item editor and are reused for skills.
- Created DataHandler/SkillData/SkillDataLoader.h/cpp - Ported logic from ZzzInfomation.cpp::OpenSkillScript() (lines 245-294) - Implements file size verification (88 bytes × 650 + 4 = 57,204 bytes) - Checksum verification using GenerateCheckSum2() with magic key 0x5A18 - BuxConvert decryption for each skill entry - UTF-8→UTF-16 string conversion for skill names via CMultiLanguage::ConvertFromUtf8() - Logs to MuEditorConsoleUI instead of MessageBox for better editor UX - Operates on global SkillAttribute array
- Created DataHandler/SkillData/SkillDataSaver.h/cpp - Automatic .bak file creation before saving - Change tracking system using X-macros for field-by-field comparison - UTF-16→UTF-8 conversion for skill names via CMultiLanguage::ConvertToUtf8() - BuxConvert encryption for each skill entry - Checksum generation using GenerateCheckSum2() with magic key 0x5A18 - Detailed change log showing old→new values for modified fields - Logs to MuEditorConsoleUI with success message and modification count - Operates on global SkillAttribute array
- Created DataHandler/SkillData/SkillDataExporter.h/cpp - X-macro driven CSV generation using SKILL_FIELDS_SIMPLE and SKILL_FIELDS_ARRAYS - UTF-8 BOM for proper encoding in Excel/LibreOffice - CSV header automatically generated from field definitions (31 columns) - UTF-16→UTF-8 conversion for skill names via WideCharToMultiByte() - Quote escaping for CSV safety (doubles quotes in skill names) - Only exports skills with non-empty names - Logs exported count to MuEditorConsoleUI - Operates on global SkillAttribute array
- Created DataHandler/SkillData/SkillDataHandler.h/cpp - Singleton pattern matching ItemDataHandler architecture - Delegates Load() to SkillDataLoader (available in all builds) - Delegates Save() and ExportToCsv() to SkillDataSaver/Exporter (editor-only) - Provides data access methods: GetSkillAttributes(), GetSkillAttribute(), GetSkillCount() - Bounds checking in GetSkillAttribute() returns nullptr for invalid indices - Non-copyable singleton with deleted copy constructor and assignment operator - Macro g_SkillDataHandler for convenient access - Operates on global SkillAttribute array
- Modified ZzzOpenData.cpp to use g_SkillDataHandler.Load() instead of OpenSkillScript() - Added include for DataHandler/SkillData/SkillDataHandler.h - Applies to ALL builds (editor and release), not just editor builds - SkillDataHandler provides cleaner abstraction and better error handling - Editor builds log to console, release builds show MessageBox errors - Old OpenSkillScript() function in ZzzInfomation.cpp now deprecated - Consistent with ItemDataHandler migration pattern
- Created MuEditor/UI/SkillEditor/SkillEditorColumns.h/cpp - X-macro-driven rendering using SkillFieldDescriptor - Delegates to generic RenderFieldByDescriptor template from FieldMetadataHelper.h - Type-specific rendering: Byte, Word, Int, DWord, Bool, WCharArray - RenderIndexColumn for skill index swapping with bounds checking (MAX_SKILLS) - UTF-8 conversion for skill names via WideCharToMultiByte/MultiByteToWideChar - Logs all changes to MuEditorConsoleUI - Works with global SkillAttribute array - Supports DWORD fields for Distance and SkillBrand
- Created MuEditor/UI/SkillEditor/SkillEditorTable.h/cpp - Case-insensitive skill name filtering with UTF-8 conversion - ImGui table with resizable/reorderable/hideable/sortable columns - Column freezing support for Index and Name columns - Virtual scrolling with ImGuiListClipper for 650 skills - Metadata-driven column setup using GetSkillFieldDescriptors() - Delegates row rendering to CSkillEditorColumns - Static RequestScrollToIndex() for navigation - Works with MAX_SKILLS (650) instead of MAX_ITEM - Updated SkillEditorColumns.cpp to include SkillEditorTable.h - add GetSkillFieldDisplayName() function to SkillFieldMetadata.h
- Created MuEditor/UI/SkillEditor/SkillEditorActions.h/cpp - Save button calls g_SkillDataHandler.Save() with changelog tracking - Export CSV button calls g_SkillDataHandler.ExportToCsv() - Metadata-driven export helpers for CSV and readable formats - GetFieldValueAsString() supports all field types including DWord - UTF-8 conversion for skill names - Created MuEditor/UI/SkillEditor/SkillEditorPopups.h/cpp - Success/failure popups for Save and Export CSV operations - Changelog display in save success popup - Static state management for popup visibility - RenderPopups() to display all active modal dialogs
- Created MuEditor/UI/SkillEditor/MuSkillEditorUI.h/cpp - Singleton pattern with g_MuSkillEditorUI define - Main ImGui window with translated "Skill Editor" title - Case-insensitive search bar for filtering skills by name - Column visibility menu with Select All/Unselect All buttons - Freeze columns checkbox (freezes Index and Name columns) - Window position clamping between toolbar and console - Integrates SkillEditorTable, SkillEditorActions, SkillEditorPopups - Default visible columns: Name, Level, Damage, Mana, Distance, Delay, Energy, Strength, Dexterity - Saves/loads column preferences via MuEditorConfig - Hover detection for input blocking
Add skill editor column visibility persistence to the configuration system.
Methods GetSkillEditorColumnVisibility() and SetSkillEditorColumnVisibility()
now allow MuSkillEditorUI to save/load column preferences.
Changes:
- Add GetSkillEditorColumnVisibility() and SetSkillEditorColumnVisibility() methods
- Add m_skillEditorColumnVisibility map member
- Update Load() to parse [SkillEditorColumnVisibility] section
- Update Save() to write [SkillEditorColumnVisibility] section
- Fix: Escape backslash in m_configPath ("MuEditor\\MuEditor.ini")
Add skill editor window rendering support to the editor core. The skill editor window is now conditionally rendered when m_bShowSkillEditor is true. Changes: - Add m_bShowSkillEditor member and IsShowingSkillEditor() accessor - Include MuSkillEditorUI.h header - Initialize m_bShowSkillEditor to false in constructor - Render skill editor window in Render() when m_bShowSkillEditor is true - Save skill editor column preferences in Shutdown()
Add skill editor button to the main toolbar next to the Item Editor button. The button toggles the skill editor window visibility. Changes: - Update RenderToolbar() and RenderToolbarFull() to accept showSkillEditor parameter - Add "Skill Editor" button in toolbar after "Item Editor" button - Update MuEditorCore to pass m_bShowSkillEditor to RenderToolbar() - Update MuEditorCenterPaneUI to render skill editor window
… (Tagalog) translations for editor UI, metadata, and game placeholders.
… UI language selector with dynamic population based on available translations.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…support; fallback to default font if unavailable.
…roved readability and maintainability.
…place existing implementations.
…emDataExporter` for consistency and reusability
ecf6cc8 to
ecaeeca
Compare
No description provided.