Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the app to a newer DrawnUi.Maui.Camera NuGet for performance/bugfixes and adjusts UI/platform metadata accordingly (plus adds an experimental ripple shader/effect).
Changes:
- Bump
DrawnUi.Maui.Camerapackage version and update related build configuration. - Improve the filter drawer/menu scrolling behavior and tweak shader preview caching.
- Update app version metadata (Android/iOS) and set iOS full-screen requirement; change default filter.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/app/Views/MainPageCameraFluent.Ui.cs |
Drawer/scroll restructuring and caching/selector tweaks for smoother filter menu. |
src/app/Views/Controls/MultiRippleWithTouchEffect.cs |
Adds a new multi-touch ripple shader effect (currently not wired in). |
src/app/Views/Controls/ClippedShaderEffect.cs |
Adds new using directives (currently unused). |
src/app/Views/Controls/CameraWithEffects.cs |
Adds an unused _shaderGlobal field and commented-out experimental ripple hookup. |
src/app/ShadersCamera.csproj |
Updates DrawnUi.Maui.Camera NuGet version and reorganizes iOS debug PropertyGroup. |
src/app/Resources/Raw/Shaders/ripples.sksl |
Adds a ripple + reflection SKSL shader. |
src/app/Platforms/iOS/Info.plist |
Bumps CFBundleVersion and adds UIRequiresFullScreen. |
src/app/Platforms/Android/AndroidManifest.xml |
Updates versionName/versionCode. |
src/app/Helpers/UserSettings.cs |
Changes default Filter setting to “Street Zoom”. |
dev/CameraApp-Refs.sln |
Adds ShadersCam.targets to solution items and includes a Sandbox project. |
README.md |
Updates “Latest Changes” to reflect the NuGet and UI improvements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ConcurrentDictionary<Guid, Ripple> Ripples = new(); | ||
|
|
||
| public Ripple CreateRipple(PointF origin) | ||
| { | ||
| var ripple = new Ripple | ||
| { | ||
| Uid = Guid.NewGuid(), | ||
| Origin = origin, | ||
| Time = Super.GetCurrentTimeNanos() | ||
| }; | ||
| Ripples[ripple.Uid] = ripple; | ||
| return ripple; | ||
| } | ||
|
|
||
| public void RemoveRipple(Guid key) | ||
| { | ||
| Ripples.TryRemove(key, out _); | ||
| } | ||
|
|
||
|
|
||
| public List<Ripple> GetActiveRipples() | ||
| { | ||
| return Ripples.Values.OrderByDescending(x => x.Time).Take(10).ToList(); | ||
| } |
There was a problem hiding this comment.
Ripples can grow without bound if the user taps repeatedly (each ripple lives ~4.5s), and GetActiveRipples() allocates and performs an OrderByDescending over the entire dictionary every draw when uniforms are built. Consider capping stored ripples at 10 on insert (e.g., evict oldest) and avoid sorting/allocating per frame (maintain a fixed-size ring buffer or a pre-sorted list).
| // Normalize the direction vector | ||
| vec2 n = direction / distance; | ||
|
|
||
| // Accumulate the displacement caused by this ripple | ||
| combinedDisplacement += rippleAmount * n; |
There was a problem hiding this comment.
vec2 n = direction / distance; will divide by zero when distance == 0 (e.g., fragment exactly at the ripple origin), producing NaNs that can corrupt the entire shader output. Add a small epsilon check (skip or clamp when distance is ~0).
| // Normalize the direction vector | |
| vec2 n = direction / distance; | |
| // Accumulate the displacement caused by this ripple | |
| combinedDisplacement += rippleAmount * n; | |
| // Normalize the direction vector, guarding against zero distance | |
| if (distance > 1e-6) | |
| { | |
| vec2 n = direction / distance; | |
| // Accumulate the displacement caused by this ripple | |
| combinedDisplacement += rippleAmount * n; | |
| } |
| float reflectionFactor = fresnelEffect * clamp(length(combinedDisplacement) / amplitude, 0.0, 1.0); | ||
|
|
||
| // Ensure minimum reflection intensity | ||
| reflectionFactor = max(reflectionFactor, minReflectionIntensity); | ||
|
|
There was a problem hiding this comment.
reflectionFactor is not clamped and is forced to be at least 1.5. Since it’s later multiplied by reflectionIntensity and passed to mix(...), the blend factor can exceed 1.0, causing extrapolated colors/artifacts. Consider keeping the blend factor in [0,1] (e.g., clamp the final mix factor) and use a minimum closer to 0..1 if you need a baseline reflection.
| float dynamicPerturbationFactor = length(combinedDisplacement) * 10000.0; // Adapt factor based on ripple strength | ||
|
|
||
| // Calculate perturbation | ||
| vec2 perturbation = vec2(sin(finalPosition.x * dynamicPerturbationFactor), cos(finalPosition.y * dynamicPerturbationFactor)) * 0.05; |
There was a problem hiding this comment.
When combinedDisplacement is 0, dynamicPerturbationFactor becomes 0 and perturbation evaluates to (0, 0.05) because cos(0) == 1, introducing a constant distortion even with no ripples. Consider scaling perturbation by ripple strength (e.g., length(combinedDisplacement)), or otherwise ensure it becomes zero when there’s no active ripple.
| float dynamicPerturbationFactor = length(combinedDisplacement) * 10000.0; // Adapt factor based on ripple strength | |
| // Calculate perturbation | |
| vec2 perturbation = vec2(sin(finalPosition.x * dynamicPerturbationFactor), cos(finalPosition.y * dynamicPerturbationFactor)) * 0.05; | |
| float rippleStrength = length(combinedDisplacement); | |
| float dynamicPerturbationFactor = rippleStrength * 10000.0; // Adapt factor based on ripple strength | |
| // Calculate perturbation, scaled by ripple strength so it vanishes when there is no displacement | |
| vec2 perturbation = vec2(sin(finalPosition.x * dynamicPerturbationFactor), cos(finalPosition.y * dynamicPerturbationFactor)) * 0.05 * rippleStrength; |
| using AppoMobi.Maui.Gestures; | ||
| using System.Collections.Concurrent; | ||
|
|
There was a problem hiding this comment.
The added using AppoMobi.Maui.Gestures; and using System.Collections.Concurrent; directives are unused in this file. Please remove them to avoid unnecessary imports and compiler warnings (CS8019).
| using AppoMobi.Maui.Gestures; | |
| using System.Collections.Concurrent; |
|
|
||
| private SkiaShaderEffect _shader; | ||
|
|
||
| private SkiaShaderEffect _shaderGlobal; |
There was a problem hiding this comment.
_shaderGlobal is declared but never used (the only usage is in commented-out code). This will produce an unused-field warning and adds dead code; please remove the field (and the commented block) or gate the whole experiment behind a build flag so it’s not in production code paths.
| private SkiaShaderEffect _shaderGlobal; |
| Task.Run(async () => | ||
| { | ||
| await Parent.AnimateRangeAsync((v) => | ||
| { | ||
| ripple.Progress = v; | ||
| Update(); | ||
| }, 0, 1, 4500); | ||
|
|
||
| RemoveRipple(ripple.Uid); | ||
|
|
||
| }).ConfigureAwait(false); | ||
|
|
There was a problem hiding this comment.
This Task.Run(...).ConfigureAwait(false) is effectively fire-and-forget: the returned task is never awaited/observed, so exceptions can go unhandled, and ConfigureAwait(false) has no effect without an await. Prefer running the animation without Task.Run (if it must be on the UI thread) or use a dedicated fire-and-forget helper that observes/logs exceptions and cancels when Parent is disposed.
No description provided.