-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[GenAI] [DRAFT] Add a offset slider to modify the placement of the video #1959
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
boweiliu
wants to merge
6
commits into
CapSoftware:main
Choose a base branch
from
boweiliu:bowei/position-offset-field
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
34a2da5
Add PositionOffsetField component
bcabb2c
Add PaddingField component and rendering updates
88783e8
Add MR_TEST_REVIEW.md
eca1e81
Wire PositionOffsetField into ConfigSidebar
2a05b14
Wire PositionOffsetField into BackgroundSettingsPopover
cb91f21
depot
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| # Depot Build — cap-web | ||
|
|
||
| Local Docker build of `apps/web` OOMs on Docker Desktop default RAM. Use Depot remote builder instead. | ||
|
|
||
| ## Prereqs | ||
|
|
||
| - `depot` CLI installed (`brew install depot/tap/depot`) | ||
| - `.env` with: | ||
| ``` | ||
| export DEPOT_DEV_API_KEY=depot_org_... | ||
| export DEPOT_DEV_ORG_ID=.. | ||
| export DEPOT_DEV_PROJECT_ID=... | ||
| ``` | ||
|
|
||
| ## Build + load into local Docker | ||
|
|
||
| ```bash | ||
| source .env | ||
| DEPOT_TOKEN=$DEPOT_DEV_API_KEY \ | ||
| depot build \ | ||
| --project $DEPOT_DEV_PROJECT_ID \ | ||
| -f apps/web/Dockerfile \ | ||
| -t cap-web:local \ | ||
| --load \ | ||
| . | ||
| ``` | ||
|
|
||
| Flags: | ||
| - `--project` — Depot project ID (org token alone insufficient). | ||
| - `--load` — pull built image into local Docker daemon as `cap-web:local`. | ||
| - Build context = repo root (Dockerfile does `COPY . .`). | ||
|
|
||
| ## Verify | ||
|
|
||
| ```bash | ||
| docker images cap-web:local | ||
| ``` | ||
|
|
||
| ## Use in compose | ||
|
|
||
| `docker-compose.yml` references `cap-web:local`. Bring up: | ||
|
|
||
| ```bash | ||
| docker compose up -d --force-recreate cap-web | ||
| ``` | ||
|
|
||
| ## Push to registry (optional) | ||
|
|
||
| ```bash | ||
| DEPOT_TOKEN=$DEPOT_DEV_API_KEY \ | ||
| depot build \ | ||
| --project $DEPOT_DEV_PROJECT_ID \ | ||
| -f apps/web/Dockerfile \ | ||
| -t ghcr.io/<org>/cap-web:<tag> \ | ||
| --push \ | ||
| . | ||
| ``` | ||
|
|
||
| ## Notes | ||
|
|
||
| - Org token (`depot_org_...`) works for `depot build` with `--project`. Does NOT work for `depot projects list` (user-scoped). | ||
| - Local `docker build` fails: `ResourceExhausted: cannot allocate memory` during Next.js build. Bump Docker Desktop RAM to 12GB+ if you want local build. | ||
| - `--no-cache` on `docker compose build` skips `cap-web` because compose entry uses `image:` not `build:`. Depot is the build path. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| # MR Review — `crates/rendering/src/lib.rs` test changes | ||
|
|
||
| Branch: `position-offset-field` vs `main`. Scope: tests in `mod tests` only. | ||
|
|
||
| ## Summary of test diff | ||
|
|
||
| | Status | Test | Notes | | ||
| |--------|------|-------| | ||
| | Removed | (none) | | | ||
| | Modified | (none) | Existing tests untouched. | | ||
| | Added | `position_offset_zero_matches_centered_offset` | Sanity check: zero offset == pre-feature baseline. | | ||
| | Added | `position_offset_shifts_image_within_available_room` | Asserts ±50 shifts image by `room * 0.5` on x and y. | | ||
| | Added | `position_offset_clamps_to_keep_image_visible` | Extreme values (±500) clamp to keep image inside frame. | | ||
| | Added | `position_offset_does_not_change_display_size` | Invariant: offset only translates, never resizes. | | ||
|
|
||
| No regressions in existing coverage. No callers/signatures of `display_offset` / `display_size` changed outwardly, so old tests still exercise the same surface. | ||
|
|
||
| ## Code under test (recap) | ||
|
|
||
| New tail in `display_layout`: | ||
|
|
||
| ```rust | ||
| let room = output_size - target_size; | ||
| let shift_x = (project.background.position_offset_x / 100.0) * room.x; | ||
| let shift_y = (project.background.position_offset_y / 100.0) * room.y; | ||
| let raw_offset = centered_offset + XY::new(shift_x, shift_y); | ||
| let max_x = room.x.max(0.0); | ||
| let max_y = room.y.max(0.0); | ||
| let offset = XY::new( | ||
| raw_offset.x.clamp(0.0, max_x), | ||
| raw_offset.y.clamp(0.0, max_y), | ||
| ); | ||
| ``` | ||
|
|
||
| Plus refactor: `display_offset` + `display_size` collapse into single `display_layout`. `display_bounds` drops `output_size` param; uses `display_offset + display_size` for end. | ||
|
|
||
| ## Correctness analysis | ||
|
|
||
| ### Refactor (non-test) sanity | ||
| - `display_bounds` previously did `output_size - display_offset` for `base_end`. That assumed symmetric centering — invalid once offset is asymmetric. New form `display_offset + display_size` is correct under any offset. | ||
| - `display_size` was previously `(output - 2*offset)`, only valid when image was centered. Now derived directly from layout. Correct. | ||
| - Fold of two functions into `display_layout` returning `(offset, size)` — same outputs, no behavior drift in the no-offset case (verified by `position_offset_zero_matches_centered_offset` and the untouched existing tests). | ||
|
|
||
| ### Logic correctness — what the math actually does | ||
| `centered_offset` is the **left/top margin** when image is centered, i.e. `room/2`. New shift = `(pct/100) * room`. So: | ||
|
|
||
| | pct | raw_offset | post-clamp | meaning | | ||
| |-----|-----------|-----------|---------| | ||
| | 0 | room/2 | room/2 | centered | | ||
| | +50 | room | room | image flush against right/bottom edge | | ||
| | -50 | 0 | 0 | flush against left/top | | ||
| | +100| 1.5·room | room (clamped) | saturated | | ||
| | -100| -room/2 | 0 (clamped) | saturated | | ||
|
|
||
| **Issue**: useful slider range is `[-50, +50]`; values past ±50 are clamped no-ops. If product spec wanted ±100 to be the extremes (typical slider UX), the divisor should be `200.0` instead of `100.0`, or shift should be `(pct/100) * (room/2)`. As-is, half the slider range is dead. The tests *encode* this behavior rather than challenge it — `position_offset_shifts_image_within_available_room` uses ±50 and asserts shift == `room * 0.5`, which mathematically matches, but documents the half-range-dead UX. **Worth confirming with design before merge.** | ||
|
|
||
| ### Test-by-test verdict | ||
|
|
||
| 1. **`position_offset_zero_matches_centered_offset`** — Correct. Guards against accidental drift when feature flag is off (offset=0). Uses auto-aspect path (no `aspect_ratio`). | ||
|
|
||
| 2. **`position_offset_shifts_image_within_available_room`** — Correct given the code. Caveats: | ||
| - Uses `aspect_ratio: Wide`, so only the fixed-aspect branch is exercised. | ||
| - Does not cover combined non-zero x AND y simultaneously. | ||
| - Does not cover intermediate values (e.g. 25%) to verify linearity. | ||
| - Hard-codes the half-range-saturation behavior (see issue above). | ||
|
|
||
| 3. **`position_offset_clamps_to_keep_image_visible`** — Correct. Tests extreme out-of-range (±500) and asserts image stays in `[0, output]`. Good. But: also worth asserting image is **at the edge** (not at center), to catch a bug where clamp is too aggressive. | ||
|
|
||
| 4. **`position_offset_does_not_change_display_size`** — Correct invariant. Only checks the fixed-aspect branch. | ||
|
|
||
| ### Coverage gaps | ||
| - **Auto-aspect-ratio branch with non-zero offset is untested.** The auto branch computes `target_size = output - centered*2` differently; the same room-shift logic should apply but isn't asserted. Test 1 covers offset=0 on auto path only — insufficient. | ||
| - **No test for `display_bounds` change.** The signature/body change isn't directly exercised; relies on integration with `ProjectUniforms::new`. | ||
| - **Sign of y-axis convention**: `position_offset_y = +50` produces `centered.y + room.y/2` (image moves *down* in frame coords). Test 2 names it `shifted_down` — consistent. Confirm UI sends positive Y for "down" or negate at boundary. | ||
| - **Float strictness**: tests use `1e-6` epsilon; `f64::EPSILON` would be tighter. Current tolerance is fine but loose. | ||
|
|
||
| ## Recommendation | ||
| - **LGTM on test correctness** (assertions match implementation). | ||
| - **Block on**: confirm whether the ±50 = full-extreme is the intended slider semantics. If not, fix code (`/200.0` or `room/2` factor), update tests accordingly. | ||
| - **Nice-to-have**: add an auto-aspect branch shift test, and a combined x+y test. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import "../../desktop/src/styles/theme.css"; | ||
| import "@cap/ui-solid/main.css"; | ||
|
|
||
| const preview: Preview = { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -108,16 +108,12 @@ impl Coord<CroppedDisplaySpace> { | |||||||||||||||||||
| resolution_base: XY<u32>, | ||||||||||||||||||||
| ) -> Coord<FrameSpace> { | ||||||||||||||||||||
| let crop = ProjectUniforms::get_crop(options, project); | ||||||||||||||||||||
| let output_size = ProjectUniforms::get_output_size(options, project, resolution_base); | ||||||||||||||||||||
| let padding_offset = ProjectUniforms::display_offset(options, project, resolution_base); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let output_size = XY::new(output_size.0, output_size.1).map(|v| v as f64); | ||||||||||||||||||||
| let display_size = ProjectUniforms::display_size(options, project, resolution_base); | ||||||||||||||||||||
|
Comment on lines
110
to
+112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this now computes the same layout twice. Since
Suggested change
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| let position_ratio = self.coord / crop.size.map(|v| v as f64); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Coord::new( | ||||||||||||||||||||
| padding_offset.coord + (output_size - padding_offset.coord * 2.0) * position_ratio, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| Coord::new(padding_offset.coord + display_size.coord * position_ratio) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new control calls
setProjecton every raw slider change, but it does not use the editor slider wrapper that pauses project history during a drag. Dragging the offset control can record many tiny intermediate states, so undo only reverts the last tick instead of the whole drag.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!