Conversation
…as-Engine into feature/point-lights
…as-Engine into feature/point-lights
…as-Engine into feature/point-lights # Conflicts: # data/shader/rtgi/temporal.csh
…om/tippesi/Atlas-Engine into feature/editor-terrain-integration
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7efafe4f37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| else if (heightMapSelection && !heightMapImage->HasData()) { | ||
| Notifications::Push({ "No heightmap loaded!", vec3(1.0f, 0.0f, 0.0f) }); |
There was a problem hiding this comment.
When heightMapSelection is non-zero and no image has been loaded yet (for example right after switching to "Load file"/"Use existing terrain" before the async update finishes), this branch dereferences heightMapImage unconditionally and can crash the editor. The condition should check that heightMapImage is non-null before calling HasData().
Useful? React with 👍 / 👎.
| p.materials.emplace_back( | ||
| FileImporter::ImportFile<Material>(m["material"]), m["color"] | ||
| ); |
There was a problem hiding this comment.
Deserialization assumes every entry in materials has a material field, but serialization intentionally omits that key for invalid handles; this makes loading fail with an exception after a user clears a material slot and saves settings. The loader needs to tolerate entries that only contain color (or filter them out) instead of unconditionally indexing m["material"].
Useful? React with 👍 / 👎.
| for (auto mat : materials) { | ||
| if (mat.first == selectedMaterial) { | ||
| pair = mat; |
There was a problem hiding this comment.
The preview generation lambda runs on a background job but reads materials/selectedMaterial through this; the UI thread can modify materials via add/delete actions while the job is running, which is an unsynchronized std::vector access and can lead to intermittent crashes or corrupted preview output. Capture a snapshot of the material state for the job instead of accessing shared mutable members.
Useful? React with 👍 / 👎.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 1 high |
| Security | 3 critical |
🟢 Metrics 279 complexity · 54 duplication
Metric Results Complexity 279 Duplication 54
TIP This summary will be updated as you push new changes. Give us feedback
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b48800b2f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| else if (!selectedMaterial.IsLoaded() && !advanced) { | ||
| Notifications::Push({ "At least one material is required!", vec3(1.0f, 0.0f, 0.0f) }); | ||
| } | ||
| else if (heightMapSelection && !heightMapImage->HasData()) { |
There was a problem hiding this comment.
This validation path dereferences heightMapImage whenever heightMapSelection is non-zero, but heightMapImage can still be null (for example, selecting "Load file" without choosing a file, or before async terrain-map extraction finishes). Pressing Generate in that state will crash instead of showing the intended warning. Check the pointer first (e.g. !heightMapImage || !heightMapImage->HasData()).
Useful? React with 👍 / 👎.
| p.materials.emplace_back( | ||
| FileImporter::ImportFile<Material>(m["material"]), m["color"] | ||
| ); |
There was a problem hiding this comment.
to_json writes material entries without a material field when the slot is empty, but this deserializer unconditionally reads m["material"]. If the user saved terrain-generator settings with an unassigned material slot, loading throws during scene-window restore and breaks config deserialization. Guard for missing keys and preserve empty slots instead of forcing import.
Useful? React with 👍 / 👎.
| else if (!editingMode && LoDDistances.size()) { | ||
| // If we are here it means we have exited editing mode | ||
| for (int32_t i = 0; i < terrain->LoDCount; i++) { | ||
| terrain->SetLoDDistance(i, LoDDistances[i]); | ||
| } |
There was a problem hiding this comment.
When editing mode is turned off, this branch restores distances from LoDDistances but never clears the cache, so it keeps running every frame and can index past the vector if the user later switches to a terrain with more LoDs than were cached. That creates an out-of-bounds read/write path and applies stale LoD values to unrelated terrains.
Useful? React with 👍 / 👎.
2 participants
No description provided.