Fix transform controls & fragment editing#833
Conversation
Micheal Parks (micheal-parks)
changed the title
🦋 Changeset detectedLatest commit: e31589e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
…ics/acdc into transform-controls-fix
| const moddedComponentNames = (mods: unknown[]): string[] => { | ||
| const names: string[] = [] | ||
| for (const mod of mods) { | ||
| if (!mod || typeof mod !== 'object') { | ||
| continue | ||
| } | ||
| for (const opValue of Object.values(mod as Record<string, unknown>)) { | ||
| if (!opValue || typeof opValue !== 'object') { | ||
| continue | ||
| } | ||
| for (const path of Object.keys(opValue as Record<string, unknown>)) { | ||
| const match = /^components\.([^.]+)/.exec(path) | ||
| if (match) { | ||
| names.push(match[1]) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return names | ||
| } |
There was a problem hiding this comment.
nit: code convention/style question, not related to these actual changes except they are a good example.
We oftentimes inline single-statement if conditions. Generally, I find it more readable, but I can see cases where it could make them harder to read, especially when nested inside loops like the one above.
Do we think this is more or less readable?
| const moddedComponentNames = (mods: unknown[]): string[] => { | |
| const names: string[] = [] | |
| for (const mod of mods) { | |
| if (!mod || typeof mod !== 'object') { | |
| continue | |
| } | |
| for (const opValue of Object.values(mod as Record<string, unknown>)) { | |
| if (!opValue || typeof opValue !== 'object') { | |
| continue | |
| } | |
| for (const path of Object.keys(opValue as Record<string, unknown>)) { | |
| const match = /^components\.([^.]+)/.exec(path) | |
| if (match) { | |
| names.push(match[1]) | |
| } | |
| } | |
| } | |
| } | |
| return names | |
| } | |
| const moddedComponentNames = (mods: unknown[]): string[] => { | |
| const names: string[] = [] | |
| for (const mod of mods) { | |
| if (!mod || typeof mod !== 'object') continue | |
| for (const opValue of Object.values(mod as Record<string, unknown>)) { | |
| if (!opValue || typeof opValue !== 'object') continue | |
| for (const path of Object.keys(opValue as Record<string, unknown>)) { | |
| const match = /^components\.([^.]+)/.exec(path) | |
| if (match) names.push(match[1]) | |
| } | |
| } | |
| } | |
| return names | |
| } |
There was a problem hiding this comment.
I'm naturally very biased towards one statement per line, which is why a lot of my stuff gets written this way. Two statements per line seems arbitrary to me intuitively. It's sort of in the same bucket as const a = 1; const b = 2; being on the same IMO. I'm not opposed to the alternative style though if y'all feel differently, I'd just recommend we make it an auto-formatting rule rather than pointing it out in nits
There was a problem hiding this comment.
yeah that is why I am asking, I am wondering if we set this up as a linter/formatter thing
3 participants
Overview
This PR fixes an issue that silently popped up during instanced rendering updates and was exposed by #821. The selected transform controls anchor themselves to Object3Ds, which no longer exist with instanced rendering. However, before the cleanup PR, ghost object3Ds were still being created via the
<Frame>component that the selected transform controls could anchor to. The cleanup PR removed those and broke transform controls. This PR switches these controls over to useWorldMatrixlike everything else, fixing the issue.When fixing this, I discovered another fragment editing bug - fixed here - that recently became apparent due to vino2 config changes. Fragment tagging was incomplete in some scenarios when reading from the robot config, causing transform changes to not trigger the isDirty flag.
More details in comments.