feat(sozo): unreal engine bindgen - #3113
Conversation
2498cdb to
9ba704d
Compare
|
Ohayo, sensei! WalkthroughThis pull request introduces support for generating Unreal Engine bindings. A new boolean flag Changes
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
crates/dojo/bindgen/src/plugins/unrealengine/mod.rs (6)
91-120: Ohayo sensei, generated header utilities look fine!
It's good to see a sensible approach for version headers and Unreal includes. If a license banner or disclaimers are needed, remember to inject them here.
245-472: Ohayo sensei, consider splitting this big header handling method.
handle_headerdoes a lot of sorting, token processing, and output generation. Extracting smaller helper functions might improve readability and maintainability.
474-620: Ohayo sensei, parse logic is thorough but watch out for maintainability!
String-based checks for model names can get unwieldy as the codebase grows. Logging any mismatch is good practice.
674-752: Ohayo sensei, contract calls generation is neat!
You might want to consider validating input ARGS or ensuring they match the expected function signature to prevent runtime errors.
754-1076: Ohayo sensei, it's a comprehensive conversion approach!
The lengthy conditionals might become cumbersome when more types appear. A more generic or data-driven approach (like a lookup table for conversions) could simplify maintenance.
1077-1121: Ohayo sensei, unified function for type conversion is powerful!
Be cautious about fallback scenarios—some type mismatch might slip through. Consider returning errors for unexpected conditions rather than silently ignoring them.Do you want me to propose a robust error-handling pattern for type mismatches?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bin/sozo/src/commands/build.rs(3 hunks)crates/dojo/bindgen/src/lib.rs(2 hunks)crates/dojo/bindgen/src/plugins/mod.rs(2 hunks)crates/dojo/bindgen/src/plugins/unrealengine/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
crates/dojo/bindgen/src/lib.rs (2)
crates/dojo/bindgen/src/plugins/unrealengine/mod.rs (1)
new(16-18)crates/dojo/bindgen/src/plugins/mod.rs (1)
new(42-44)
crates/dojo/bindgen/src/plugins/unrealengine/mod.rs (2)
crates/dojo/bindgen/src/lib.rs (1)
compare_tokens_by_type_name(230-234)crates/dojo/bindgen/src/plugins/mod.rs (1)
generate_code(148-148)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (15)
crates/dojo/bindgen/src/lib.rs (2)
19-19: Ohayo sensei, nice addition of the import!
This import aligns perfectly with the newly introduced UnrealEngine plugin functionality.
105-105: Ohayo sensei, good extension of the plugin manager!
IncludingBuiltinPlugins::UnrealEngineneatly integrates the Unreal Engine plugin into the existing framework.bin/sozo/src/commands/build.rs (3)
37-40: Ohayo sensei, new field for Unreal bindings looks consistent!
Adding thisunrealengineboolean follows the same pattern as other flags liketypescriptandunity. Good job!
137-140: Ohayo sensei, conditionally pushing the UnrealEngine plugin is clean!
Everything integrates well with other plugin flags. Ensure to test the build flow with--unrealengineto confirm correctness.
225-225: Ohayo sensei, the default value is consistent with existing flags!
Settingunrealengineto false by default aligns with user expectations.crates/dojo/bindgen/src/plugins/mod.rs (3)
16-16: Ohayo sensei, module exposure is straightforward!
Declaring theunrealenginemodule here is a clean choice to keep the code organized.
22-22: Ohayo sensei, UnrealEngine variant extends the plugin enum nicely!
This strengthens the plugin ecosystem with minimal disruption.
32-32: Ohayo sensei, adding the UnrealEngine display branch is on point!
This ensuresBuiltinPlugins::UnrealEnginecan be stringified and recognized easily.crates/dojo/bindgen/src/plugins/unrealengine/mod.rs (7)
1-3: Ohayo sensei, imports look good!
The chosen crates and standard library collections are appropriate for generating Unreal Engine bindings.
12-18: Ohayo sensei, basic plugin struct is neat!
DefiningUnrealEnginePluginas an empty struct is clean. Just be mindful if plugin configuration or state is needed in the future.
135-167: Ohayo sensei, struct formatting logic is straightforward!
DefiningUPROPERTYfields aligns with Unreal’s reflection system. Great job ensuring correct type mapping withmap_type.
169-188: Ohayo sensei, enum generation is well handled!
Mapping the Cairo enum to aUENUM(BlueprintType)is a solid approach. Confirm that the 8-bit backing type (uint8) is sufficient for all enumerations.
190-243: Ohayo sensei, yourformat_modelapproach is consistent with Unreal conventions!
Instantiating withUCLASS(BlueprintType)and inheriting fromUDojoModelkeeps your code consistent. Nicely done.
622-672: Ohayo sensei, policies generator is straightforward!
Harvesting system tokens and formatting them for the controller function is logically consistent.
1122-1272: Ohayo sensei, concurrency usage is promising but watch for shared state!
You store a global staticAGeneratedHelpers::Instance. If multiple threads modify that, you could have a race condition. Confirm there's a single instance usage or add synchronization.
feat: generate custom types wrapper feat: handle policies fix: policies format fix: types issue feat: add convert to felt string fix: format fix: struct not showing in header file fix: enhance code quality fix: style fix: refactor code fix: style
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/dojo/bindgen/src/plugins/unrealengine/mod.rs (4)
190-206: Ohayo sensei, the doc comment here mentions formatting into a “C# struct” using “C# and unity SDK types,” but the code actually generates C++ code for Unreal. Updating these docstrings to reflect Unreal Engine usage will help maintain clarity.
979-981: Ohayo sensei, the comment “// Implémenter la logique de conversion d’array” indicates an unfinished feature. Without array deserialization logic, nested arrays will fail to convert. Consider implementing this soon or returning an error for array conversion attempts.
208-243: Ohayo sensei, the approach to generating “UDojoModel” classes and property fields is straightforward, but note that large models might become unwieldy if they have many fields. Consider refactoring into smaller components or partial classes for improved maintainability.
754-926: Ohayo sensei, your “TypeConverter” and related logic handle many primitive types with thorough coverage, but try to keep conversions consistent. If advanced features like partial field loading or skipping certain fields are needed, consider a more modular design.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/dojo/bindgen/src/plugins/unrealengine/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crates/dojo/bindgen/src/plugins/unrealengine/mod.rs (2)
crates/dojo/bindgen/src/lib.rs (1)
compare_tokens_by_type_name(230-234)crates/dojo/bindgen/src/plugins/mod.rs (3)
new(42-44)join(87-89)generate_code(148-148)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
crates/dojo/bindgen/src/plugins/unrealengine/mod.rs (3)
42-44: Ohayo sensei, consider returning an error or using a fallback instead of panicking. Panics can abruptly halt the process and might cause undesired crashes in production environments, especially for unhandledOptiontypes. This feedback has been raised before and still applies.
681-685: Ohayo sensei, the placeholder"COPY_ADDRESS_HERE"in your contract addresses is a reminder that these need to be replaced with valid addresses or dynamically retrieved during runtime. Otherwise, calls to these addresses won’t succeed.
245-472: Ohayo sensei, be aware that you are sorting structs and enums before processing them to ensure deterministic output. This is a good practice, but if new or nested struct dependencies appear, you may need additional checks to prevent incomplete or out-of-order generations.
| "tuple" => { | ||
| panic!("Tuple type not handled"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei, similarly to Option, panicking on unhandled tuple types may cause runtime interruptions. Converting panics to error results or skipping unsupported types gracefully is a healthier design pattern.
909e2ec to
8f31e2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/dojo/bindgen/src/plugins/unrealengine/mod.rs (4)
20-63: Ohayo sensei, thorough type mapping with minor edge cases.Your
map_typefunction covers a broad range of Cairo types, neatly mapping them to UE-friendly equivalents. Beyond the unresolvedOptionandtuplepanics, the rest is cohesive.
245-472: Ohayo sensei, careful data processing inhandle_header!This function ensures that structs and enums are processed in a deterministic order before generating code. The code is large, but each step is purposeful. If you find it grows further, consider splitting out dependency resolution from code emission to keep it maintainable.
674-752: Ohayo sensei, well-defined contract calls!
generate_contract_addresses_and_calls_functionsneatly composes contract addresses with call wrappers. The function body is succinct and keeps blueprint usage in mind. If needed, factor out the logic that builds argument arrays for improved testability.
754-921: Ohayo sensei, type conversions are comprehensive but quite large.
generate_converterssystematically accounts for custom structs and enums, including array conversions.- The big chain of
if constexprinConvertToFeltHexais correct but could be more maintainable if split into smaller specialized functions.- Marking places like
// Implémenter la logique de conversion d'arraywith a TODO might guide future devs.Consider modularizing to lighten the function size.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
bin/sozo/src/commands/build.rs(3 hunks)crates/dojo/bindgen/src/lib.rs(2 hunks)crates/dojo/bindgen/src/plugins/mod.rs(2 hunks)crates/dojo/bindgen/src/plugins/unrealengine/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/dojo/bindgen/src/lib.rs
- bin/sozo/src/commands/build.rs
🧰 Additional context used
🧬 Code Definitions (2)
crates/dojo/bindgen/src/plugins/mod.rs (1)
crates/dojo/bindgen/src/plugins/typescript/writer.rs (2)
write(22-96)write(115-183)
crates/dojo/bindgen/src/plugins/unrealengine/mod.rs (2)
crates/dojo/bindgen/src/lib.rs (1)
compare_tokens_by_type_name(230-234)crates/dojo/bindgen/src/plugins/mod.rs (1)
generate_code(148-148)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (13)
crates/dojo/bindgen/src/plugins/mod.rs (3)
16-16: Ohayo sensei, new module added!This addition cleanly declares the
unrealenginemodule, aligning well with the existing plugin structure.
22-22: Ohayo sensei, fresh enum variant looks good!Adding
UnrealEnginetoBuiltinPluginsfollows the established pattern for new built-in plugins.
32-32: Ohayo sensei, Display implementation is consistent!This match arm for
UnrealEnginecompletes thefmt::Displaycoverage without issues.crates/dojo/bindgen/src/plugins/unrealengine/mod.rs (10)
42-44: Ohayo sensei, consider removing panic for unsupported Option type.This matches a previous suggestion about gracefully handling or returning an error instead of panicking.
54-55: Ohayo sensei, consider removing panic for unsupported tuple type.This also revisits a past recommendation: returning a descriptive error (or skipping the token) improves resilience.
1-19: Ohayo sensei, nice plugin struct!The
UnrealEnginePluginstruct is straightforward, and instantiating it with thenewmethod stays consistent with the codebase’s plugin pattern. This design is minimal and easy to extend.
91-133: Ohayo sensei, well-structured import logic!The
generated_header(),header_imports(), andcppfile_imports()functions produce consistent, single-purpose code. This modular approach helps keep Unreal includes centralized, preventing duplication.
135-243: Ohayo sensei, robust struct/enum/model formatting!
format_structandformat_enummap Cairo data cleanly to Unreal patterns (USTRUCT,UENUM).to_pascal_caseis helpful, though consider edge cases with repeated underscores or non-alphabetic characters.format_modelencapsulates model definitions withUCLASSeffectively, which is a plus for blueprint integration.Overall, this is well architected.
474-621: Ohayo sensei, good approach to parse models!
generate_parse_models_functionssystematically iterates models and inserts them into Unreal objects. The final asynchronous broadcast withOnDojoModelUpdatedis a neat, blueprint-friendly design. Keep an eye on memory usage for large datasets, but otherwise this approach is solid.
622-672: Ohayo sensei, policy generation is straightforward!
generate_policiesand the partial usage inControllerGetAccountOrConnectcreate clarity for how system calls are filtered. The code is concise, though validation or logging for unknown systems might further help debugging.
1128-1335: Ohayo sensei,handle_cppfileorganizes large generation tasks well!This method stitches everything together: converters, parse functions, policy logic, and addresses. Great job unifying these parts. For bigger expansions, watch out for function length and diffuse it if needed.
1337-1351: Ohayo sensei, recursive token checks are solid!
check_token_in_recursivelyis a helpful helper for nested composite structures. It's tidy and should be easy to maintain.
1353-1376: Ohayo sensei, plugin implementation is consistent!Implementing
BuiltinPluginforUnrealEnginePluginfollows the same pattern as other plugins, ensuring code remains cohesive and future-proof.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3113 +/- ##
==========================================
- Coverage 56.82% 55.74% -1.09%
==========================================
Files 442 443 +1
Lines 61561 62749 +1188
==========================================
- Hits 34981 34977 -4
- Misses 26580 27772 +1192 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| println!("Generating header: GeneratedHelpers.h"); | ||
| let code = self.handle_header(&models, &contracts); | ||
| out.insert("GeneratedHelpers.h".into(), code.as_bytes().to_vec()); |
There was a problem hiding this comment.
Shouldn't be easier to find with DojoHelpers? Or it's a convention in unreal engine?
| } | ||
| } | ||
|
|
||
| format!( |
There was a problem hiding this comment.
Combining raw string (r#) and format! could avoid escaping the double quotes.
| }} | ||
| ", | ||
| namespace = Self::to_pascal_case(&get_namespace_from_tag(&model.tag)), | ||
| model_name = model |
There was a problem hiding this comment.
Even if it's not a critical code, we may extract some temporary variable inside the map to avoid iterating several times on the same data structure.
| " | ||
| #include \"GeneratedHelpers.h\" | ||
| #include <string> | ||
| #include <iomanip> | ||
| #include <sstream> | ||
| #include <memory> | ||
| #include \"Async/Async.h\" | ||
|
|
||
| " |
There was a problem hiding this comment.
| " | |
| #include \"GeneratedHelpers.h\" | |
| #include <string> | |
| #include <iomanip> | |
| #include <sstream> | |
| #include <memory> | |
| #include \"Async/Async.h\" | |
| " | |
| r#" | |
| #include "GeneratedHelpers.h" | |
| #include <string> | |
| #include <iomanip> | |
| #include <sstream> | |
| #include <memory> | |
| #include "Async/Async.h" | |
| "# |
Favorise the use of raw string for easier maintenance of the generated code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/dojo/bindgen/src/plugins/unrealengine/mod.rs (3)
550-615: Ohayo sensei, this function is quite large and may benefit from splitting into smaller helpers.
Refactoring it into multiple functions could improve readability and make testing easier.
961-967: Ohayo sensei, there’s a placeholder comment for array conversion logic.
Would you like help completing this logic for converting C array data intoTArray?
1314-1328: Ohayo sensei, consider cycles that might cause infinite recursion incheck_token_in_recursively.
If a composite references itself, this function could never return. Let me know if you’d like assistance in adding a safeguard for cyclic references.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/dojo/bindgen/src/plugins/unrealengine/mod.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
crates/dojo/bindgen/src/plugins/unrealengine/mod.rs (3)
41-43: Ohayo sensei, consider a gentler approach than panics for unsupportedOptiontypes.
This abrupt panic could interrupt a larger application at runtime. Returning an error or skipping unsupported types would be more robust.
53-55: Ohayo sensei, consider a gentler approach than panics for unhandledtupletypes.
Similar toOption, these panics can stop the entire process unexpectedly. Returning an error result or skipping the type is generally safer.
1205-1211: Ohayo sensei, ensure safe object handling when capturingthisin asynchronous callbacks.
If the actor is destroyed, this captured pointer could be invalid, leading to potential crashes. Please confirm that the actor remains valid at callback time.
2 participants
Description
Generate bindings for the Unreal Engine SDK
Checklist
scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit