feat: C# scripting improvements

[NicusorN5]

Tasklist:

• Custom windows for object
• Scintilla support
• C# demo with SkyColor object
• Update FFIs

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 330 complexity · 4 duplication

Metric	Results
Complexity	330
Duplication	4

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

_{TIP This summary will be updated as you push new changes.}

[codacy-production]

Pull Request Overview

This PR introduces a reflection-based custom editor system and C# scripting improvements, but it is currently not up to standards. There are critical issues that prevent merging: a compilation error in CSharpScriptEditor.cpp due to a missing constructor, and the inclusion of a blocking WinMain call in the test suite which will hang automated CI/CD environments. Additionally, hardcoded absolute paths in the solution and project files break build portability.

From a quality perspective, the manual memory management within Win32 callbacks is fragile and likely to cause leaks or double-free errors. The implementation of the C# script editor and the reflection factory (EditObject) are identified as high-complexity areas with zero test coverage, leaving all acceptance criteria regarding error handling and verification unaddressed.

About this PR

• The integration of Scintilla support appears incomplete; while loading logic exists, the functional depth of the text editor is limited. Furthermore, the manual memory management pattern used across the new Win32 dialogs is fragile. Consider using smart pointers or RAII wrappers to manage the lifecycle of objects like CSharpScript within UI callbacks.

Test suggestions

• Invoke EditObject for an object without the CustomWindow attribute to ensure it defaults to EditObjectDialog.
• Invoke EditObject for an object with a CustomWindow attribute and verify the correct exported function is called.
• Verify error handling in EditObject when the specified CustomWindow function cannot be found in any loaded modules.
• Verify CSharpScriptEditor correctly updates CSharpScript object fields (Name, Class, Enabled) upon OK command.
• Test Scintilla initialization fallback logic when Scintilla.dll is missing from the environment.
• Add unit tests for CSharpScriptEditor and EditObject to address identified logic complexity and coverage gaps.

Prompt proposal for missing tests

Consider implementing these tests if applicable:
1. Invoke EditObject for an object without the CustomWindow attribute to ensure it defaults to EditObjectDialog.
2. Invoke EditObject for an object with a CustomWindow attribute and verify the correct exported function is called.
3. Verify error handling in EditObject when the specified CustomWindow function cannot be found in any loaded modules.
4. Verify CSharpScriptEditor correctly updates CSharpScript object fields (Name, Class, Enabled) upon OK command.
5. Test Scintilla initialization fallback logic when Scintilla.dll is missing from the environment.
6. Add unit tests for CSharpScriptEditor and EditObject to address identified logic complexity and coverage gaps.

_{TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback}

[NicusorN5]

Picture of current progress

[codacy-production]

Pull Request Overview

This PR successfully introduces a reflection-based mechanism for custom object editors and integrates Scintilla for C# script editing. However, several critical issues must be addressed before merging. Most notably, a pointer is incorrectly compared to an integer constant (IDCANCEL) in EditorWindow.cpp, which will likely lead to logic failures when closing dialogs. Additionally, hardcoded absolute paths in the project files will break builds for other contributors.

While Codacy marks the PR as up to standards, CSharpScriptEditor.cpp is flagged as a complex file with no test coverage. This is concerning given the manual memory management and DLL-loading logic present in that file. There are also some implementation gaps regarding error handling for missing libraries (Lexilla/Scintilla) and potential performance overhead in the module resolution logic.

About this PR

• The dynamic window resolution logic and data binding in the C# editor lack automated tests. Consider adding tests to verify that custom windows resolve correctly and that script data is persisted accurately.
• The PR contains incomplete tasks according to the description (SkyColor demo and FFI updates). Ensure these are completed or tracked as follow-up items.

Test suggestions

• Verify EditObject falls back to EditObjectDialog when no CustomWindow attribute is present on a reflected object.
• Verify EditObject correctly resolves and executes the CreateCSharpEditorWindow function from the loaded module.
• Verify Scintilla editor initializes correctly and applies C# keyword highlighting via Lexilla.
• Verify the C# script editor saves name, class, and enabled state changes back to the script object upon clicking OK.
• Verify keyword autocompletion appears in the Scintilla editor after typing characters.
• Implement unit/integration tests for CSharpScriptEditor.cpp to cover Scintilla initialization and DLL fallback logic.

Prompt proposal for missing tests

Consider implementing these tests if applicable:
1. Verify EditObject falls back to EditObjectDialog when no CustomWindow attribute is present on a reflected object.
2. Verify EditObject correctly resolves and executes the CreateCSharpEditorWindow function from the loaded module.
3. Verify Scintilla editor initializes correctly and applies C# keyword highlighting via Lexilla.
4. Verify the C# script editor saves name, class, and enabled state changes back to the script object upon clicking OK.
5. Verify keyword autocompletion appears in the Scintilla editor after typing characters.
6. Implement unit/integration tests for CSharpScriptEditor.cpp to cover Scintilla initialization and DLL fallback logic.

_{TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback}

[codacy-production]

_{🔴 HIGH RISK}

Directly comparing a pointer to IDCANCEL is logically incorrect. This check will likely fail to detect a cancelled dialog as intended. If the dialog was cancelled, the function should return nullptr and this check should be updated to if (obj == nullptr).

[NicusorN5]

to be fixed later

[codacy-production]

Pull Request Overview

The PR introduces significant C# scripting and error-handling improvements. While Codacy reports the code is up to standards, the review identified several critical issues that should be addressed before merging. Specifically, there is a memory leak in the script compilation logic and logic errors in the Scintilla-based editor that will cause crashes or break functionality in multi-window scenarios.

Furthermore, the use of absolute paths to specific drive letters (D:) in tests and project files prevents this PR from being portable or buildable in CI environments. A substantial portion of the PR involves a logging and exception system refactor which was not detailed in the PR description. Lastly, high-complexity files like CSharpScriptEditor.cpp and ScriptManager.cs lack corresponding unit tests, leaving critical FFI and compilation logic unverified.

About this PR

• Hardcoded absolute file paths are used in both the test source code and the demo project file. These will cause build failures on other developer machines and CI/CD runners that do not share the exact disk structure of the author.
• The CSharpScriptEditor lacks robust error handling for external library loading (Scintilla/Lexilla). If these DLLs fail to load, the editor proceeds to call null function pointers, which will lead to application crashes.

1 comment outside of the diff

3DRadSpace/Projects/RGB Fade.3drsp

_{line 67 🟡 MEDIUM RISK}
The 'Script path' should be relative to the project file or the engine root to ensure portability across different workstations.

Test suggestions

• Verify that 'EditObject' factory successfully finds and invokes a custom window function from a different DLL module.
• Verify that 'Logging::Exception' captures a valid stacktrace including line numbers and function names.
• Verify C# script compilation via the FFI bridge returns correct success/failure codes based on code validity.
• Confirm that the integrated 'RGB Fade' project loads and executes the C# script during an engine test run.

Prompt proposal for missing tests

Consider implementing these tests if applicable:
1. Verify that 'EditObject' factory successfully finds and invokes a custom window function from a different DLL module.
2. Verify that 'Logging::Exception' captures a valid stacktrace including line numbers and function names.
3. Verify C# script compilation via the FFI bridge returns correct success/failure codes based on code validity.

Low confidence findings

• The PR description focuses on C# scripting but omits the major overhaul of the engine's exception and logging system which touches nearly every module. This should be documented for better auditability.

_{TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback}

[codacy-production]

Pull Request Overview

While this PR successfully implements significant features including Scintilla integration and physics FFI expansion, several high-risk issues must be addressed before merging. Most critical are a logic flaw in the CallCSFunction template that will cause compiler errors for void functions and a memory ownership bug in the C# physics wrappers that likely leads to double-free crashes during garbage collection.

Furthermore, the PR introduces complex, untested code in files like CSharpScriptEditor.cpp and several physics FFI modules. The test suite is currently non-portable due to hardcoded local file paths, and there are significant gaps in acceptance criteria verification—specifically regarding the discovery of custom windows and the singleton behavior of the ObjectRegistry across DLL boundaries. Refactoring is also needed to eliminate code duplication in the collider hierarchy and script management logic.

About this PR

• The PR scope is exceptionally large, combining major feature additions with a global refactoring of the logging system and exception handling. This increases the risk of regressions across the engine.

2 comments outside of the diff

3DRadSpace/3DRadSpace_CSharp/Internal/ScriptManager.cs

_{line 65 🟡 MEDIUM RISK}
The ScriptManager is not thread-safe. Use Interlocked.Increment for _nextScriptId and thread-safe collections for _loadedScripts to prevent corruption during parallel initialization.

3DRadSpace/3DRadSpace_CSharp/Physics/ICollider.cs

_{line 143 🟡 MEDIUM RISK}
Property wrappers for physics parameters (Mass, Damping, Friction) are duplicated across the collider hierarchy. These should be moved into the base ICollider or IStaticCollider classes.

Test suggestions

• Missing: Verify that the EditObject function correctly identifies and loads a custom window procedure from a plugin DLL using the CustomWindow attribute.
• Missing: Test the C# ScriptManager.CompileScript entry point to ensure it correctly validates script files without permanently loading them.
• Missing: Verify that the new FFI calls for ICharacterController (Move, Jump, Gravity) correctly interface with the underlying PhysX implementation.
• Missing: Confirm that Exception objects correctly capture and format std::stacktrace data during instantiation.
• Missing: Ensure the ObjectRegistry singleton returns the same vector instance across different loaded modules.
• Missing: Unit/Integration tests for CSharpScriptEditor.cpp (high complexity gap)
• Missing: Unit/Integration tests for IJoint.cpp (high complexity gap)
• Missing: Unit/Integration tests for IDynamicCollider.cpp (high complexity gap)
• Missing: Unit/Integration tests for ICharacterController.cpp (high complexity gap)

Prompt proposal for missing tests

Consider implementing these tests if applicable:
1. Missing: Verify that the EditObject function correctly identifies and loads a custom window procedure from a plugin DLL using the CustomWindow attribute.
2. Missing: Test the C# ScriptManager.CompileScript entry point to ensure it correctly validates script files without permanently loading them.
3. Missing: Verify that the new FFI calls for ICharacterController (Move, Jump, Gravity) correctly interface with the underlying PhysX implementation.
4. Missing: Confirm that Exception objects correctly capture and format std::stacktrace data during instantiation.
5. Missing: Ensure the ObjectRegistry singleton returns the same vector instance across different loaded modules.
6. Missing: Unit/Integration tests for CSharpScriptEditor.cpp (high complexity gap)
7. Missing: Unit/Integration tests for IJoint.cpp (high complexity gap)
8. Missing: Unit/Integration tests for IDynamicCollider.cpp (high complexity gap)
9. Missing: Unit/Integration tests for ICharacterController.cpp (high complexity gap)

Low confidence findings

• There is no verification that the ObjectRegistry singleton maintains state correctly across DLL boundaries (Core vs Plugins). This is a critical requirement for plugin support.

_{TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback}