Put the visgroup rename, variant delete and power change on screen - #643
Conversation
Three operations were implemented and had no entry point outside the suite. Each was the missing half of a feature whose other half was in the dock. rename_visgroup() had a collision check and a rewrite of every member's metadata, and the dock could do everything to a visgroup except rename it. There is a Rename button beside Delete now, and a name already taken is refused rather than merging two visgroups. remove_variant() had no caller, so a prefab's variant list was append-only while the library's own header said the context menu could delete one. Remove Variant is on that menu now and does not offer base. The Power spin was read once, at creation, so a cliff sculpted at 9x9 reached 17x17 only through Destroy and Create. set_power() resamples the old grid into the new one, and an Apply button calls it.
github-actions
Bot
added
area: build
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: b86811ad4b
ℹ️ 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".
| ## The system resamples the old grid into the new one, which is the whole reason | ||
| ## this is not Destroy and Create (#615). | ||
| func set_displacement_power(brush_id: String, face_index: int, power: int) -> bool: | ||
| var ok: bool = displacement_system.set_power(brush_id, face_index, power) |
There was a problem hiding this comment.
When the selected displacement contains alpha painting or custom offset directions, this newly reachable call silently destroys that data: HFDisplacementSystem.set_power() initializes a fresh displacement and resamples only distances, leaving alphas reset to zero and offsets empty. Applying a different power can therefore erase material blending and alter geometry even though the UI reports that the sculpt was kept; the other per-vertex channels need to be resampled as well.
Useful? React with 👍 / 👎.
| if not prefab.remove_variant(variant_name): | ||
| show_toast('"%s" is not a variant that can be removed' % variant_name, 2) | ||
| return | ||
| prefab.save_to_file(prefab_path) |
There was a problem hiding this comment.
When a placed prefab instance currently uses the variant being removed, this only rewrites the source file and leaves its PrefabInstanceRecord.variant_name pointing at a name that no longer exists. A later live-link propagation substitutes base geometry while retaining the invalid name, and push_instance_to_source() can recreate the supposedly deleted variant; either migrate affected instances to base or refuse deletion while the variant is referenced.
Useful? React with 👍 / 👎.
| "Rename Visgroup", "rename_visgroup", [current_name, new_name] | ||
| ) | ||
| refresh_visgroup_ui(dock) | ||
| reselect_visgroup_row(dock, row) |
There was a problem hiding this comment.
When the renamed visgroup is not the last entry, renaming removes its old dictionary key and appends the new key, so rebuilding the list changes its row. Reselecting the old numeric row highlights a different visgroup; the next Add Sel, Rem Sel, or Delete can then affect that group instead. Locate and select new_name after the refresh rather than reusing the stale index.
Useful? React with 👍 / 👎.
1 participant
Fixes #615
Three level-editing operations were implemented, carefully, and had no entry point outside the GUT suite. Each was the missing half of a feature whose other half was already in the dock, and each left the mapper doing destructive busywork instead.
The three
A visgroup could not be renamed.
rename_visgroup()carried a collision check and a rewrite of every member'svisgroupsmetadata. The dock could create a visgroup, delete one, add and remove a selection and toggle visibility. Someone who named oneroofand later wantedroof_upperhad to make a new one, re-add every member and delete the old. There is a Rename button beside Delete now.The collision is checked before committing rather than left to the subsystem's refusal, because
_commit_state_action()calls a method by name and cannot see a return value, so a refused rename would otherwise push an undo step for an edit that never happened.A prefab variant could not be deleted. The rest of the feature was fully wired: the context menu adds one, the library shows
[N variants], Ctrl+Shift+V cycles them. Only removal was missing, so the list was append-only while the file's own header comment claimed the context menu could delete one. Remove Variant is on that menu now and does not offerbase.A displacement's power could not be changed after creation. The Power spin was read once, at creation, so its tooltip described a choice that was final: a cliff sculpted at 9x9 could only reach 17x17 through Destroy and Create, which throws the sculpt away.
set_power()resamples the old grid into the new one by bilinear interpolation and was written for exactly this. An Apply button beside the spin now calls it; Create still reads the same spin for a new displacement.Why the existing tests did not catch this
They are the reason it survived.
rename_visgroup()had seven assertions intest_visgroup_system.gd,remove_variant()had two,set_power()had one. All green, all calling the subsystem directly, none of them able to notice that nothing else did.So
tests/test_unreachable_operations_reachable.gdgoes through the dock the way a click does —visgroup_rename_btn.pressed.is_connected(...),dock._on_prefab_variant_remove_requested(...)— rather than calling the subsystem again.I stashed the source changes and ran the new file against unmodified code: 0 passing, 11 failing. That is the check that matters for this issue.
Docs
CHANGELOG, and three places in the user guide. One of them, the Power setting, already described the resampling as though it worked — same shape as the prefab library's header comment. In both cases the documentation had been written from the implementation rather than from the product.
Also corrected the library's header comment, which listed context-menu actions it did not have.