v0.4.26: keep legacy 5-arg /videoPlace alongside new volume form

Reviewer flagged v0.4.25 broke existing command blocks by inserting
'volume' as a required arg between height and url. Split into two
Brigadier branches: legacy 5-arg defaults volume=50, muted=false; new
6-arg keeps the -1=mute shortcut. Legacy branch uses a single-token url
arg so '50 https://...' parses as the new (int) branch, not as a legacy
url starting with a digit.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

gradle.properties

@@ -5,7 +5,7 @@ org.gradle.configuration-cache=false
 
 # Mod
 mod_id=video_player
-mod_version=0.4.20
+mod_version=0.4.26
 maven_group=com.ejclaw.videoplayer
 archives_base_name=video_player
 

src/main/java/com/ejclaw/videoplayer/VideoPlayerClient.java

@@ -20,6 +20,7 @@ import net.fabricmc.fabric.api.event.player.AttackBlockCallback;
 import net.minecraft.client.Minecraft;
 import net.minecraft.client.player.LocalPlayer;
 import net.minecraft.core.BlockPos;
+import net.minecraft.sounds.SoundSource;
 import net.minecraft.world.InteractionResult;
 import net.minecraft.world.phys.Vec3;
 
@@ -89,17 +90,26 @@ public class VideoPlayerClient implements ClientModInitializer {
      * Distance is measured from the player's eye to the <em>panel center</em>, not the anchor
      * block corner — for a 4×4 panel the corner is ~2 blocks off from where the screen visually
      * sits, which made the audio feel like it was off to the side.
+     *
+     * <p>Gain is also gated by the Minecraft sound options so the in-game sliders work as
+     * expected: vanilla {@code SoundEngine.calculateVolume} multiplies by master × category, so
+     * we do the same with {@link SoundSource#PLAYERS} as the category. Result: dragging the
+     * "Players" slider in Options → Music & Sounds attenuates video audio just like other
+     * player sounds, and "Master" still gates everything.
      */
     private static void updateDistanceGains(Minecraft client) {
         LocalPlayer p = client.player;
         if (p == null || client.level == null) return;
         Vec3 eye = p.getEyePosition();
+        float masterVol = client.options.getSoundSourceVolume(SoundSource.MASTER);
+        float playersVol = client.options.getSoundSourceVolume(SoundSource.PLAYERS);
+        float categoryScale = masterVol * playersVol;
         for (BlockPos pos : VideoPlayback.activePositions()) {
             if (!(client.level.getBlockEntity(pos) instanceof VideoAnchorBlockEntity be)) continue;
             Vec3 center = be.panelCenter();
             double d = center.distanceTo(eye);
             float attenuation = (float) Math.max(0.0, Math.min(1.0, 1.0 - d / 16.0));
-            float gain = be.isMuted() ? 0F : be.getVolume() * attenuation;
+            float gain = be.isMuted() ? 0F : be.getVolume() * attenuation * categoryScale;
             VideoPlayback.setGain(pos, gain);
         }
     }

src/main/java/com/ejclaw/videoplayer/block/VideoAnchorBlockEntity.java

@@ -3,7 +3,11 @@ package com.ejclaw.videoplayer.block;
 import com.ejclaw.videoplayer.registry.VideoPlayerBlockEntities;
 import net.minecraft.core.BlockPos;
 import net.minecraft.core.Direction;
+import net.minecraft.core.HolderLookup;
 import net.minecraft.nbt.CompoundTag;
+import net.minecraft.network.protocol.Packet;
+import net.minecraft.network.protocol.game.ClientGamePacketListener;
+import net.minecraft.network.protocol.game.ClientboundBlockEntityDataPacket;
 import net.minecraft.world.level.block.entity.BlockEntity;
 import net.minecraft.world.level.block.state.BlockState;
 import net.minecraft.world.level.storage.ValueInput;
@@ -123,6 +127,35 @@ public class VideoAnchorBlockEntity extends BlockEntity {
         out.putBoolean("autoplay", autoplay);
     }
 
+    /**
+     * Vanilla chunk-load BE sync. The base implementation returns an empty tag, which means
+     * when a client first sees this BE (chunk loads or player walks into range) it gets default
+     * values — url="" in particular makes the renderer no-op and the panel appears invisible.
+     *
+     * <p>Returning {@link #toNbt()} here carries the custom fields in the vanilla packet, so
+     * we don't depend on the {@code SyncAnchorPayload} arriving before the chunk's block-update
+     * packet (there's a race: {@code level.setBlock} queues a deferred chunk broadcast while
+     * {@code ServerPlayNetworking.send} writes immediately; if the payload wins, the client
+     * drops it because the BE doesn't exist yet, then the chunk packet creates the BE with
+     * defaults). It also fixes "player walks far away and comes back" — that path has no
+     * SyncAnchorPayload at all, just vanilla chunk re-sync.
+     */
+    @Override
+    public CompoundTag getUpdateTag(HolderLookup.Provider provider) {
+        return toNbt();
+    }
+
+    /**
+     * Triggers a {@link ClientboundBlockEntityDataPacket} whenever the chunk tracker decides
+     * this BE needs to push an update. Default implementation returns {@code null} (no packet
+     * sent on BE change). Combined with {@link #getUpdateTag} above, every BE-state change a
+     * client sees carries the full config.
+     */
+    @Override
+    public Packet<ClientGamePacketListener> getUpdatePacket() {
+        return ClientboundBlockEntityDataPacket.create(this);
+    }
+
     @Override
     protected void loadAdditional(ValueInput in) {
         super.loadAdditional(in);

src/main/java/com/ejclaw/videoplayer/client/playback/VideoCache.java

@@ -233,6 +233,33 @@ public final class VideoCache {
 
     // -- internals -----------------------------------------------------------------------
 
+    /**
+     * Publish {@code path} into {@link #READY} for {@code url}, but only if the cache
+     * epoch hasn't moved since {@code startEpoch} was captured. Wraps the
+     * pre-check / put / post-check / rollback dance so every publish site uses the
+     * same race-free pattern instead of re-implementing it.
+     *
+     * <p>Race coverage: if {@link #clearAll()} runs at any point between the
+     * pre-check and the post-check, the post-check sees the epoch bump and we
+     * roll back — {@link Map#remove(Object, Object)} is a compareAndRemove so we
+     * don't clobber a legitimate concurrent put under the same key, and we delete
+     * the on-disk file because clearAll's directory scan may have missed it (the
+     * file may not have existed yet when clearAll ran).
+     *
+     * @return {@code true} if the entry is now published; {@code false} if the
+     *         caller's work was cancelled and {@code path} has been cleaned up.
+     */
+    private static boolean publishIfNotCancelled(String url, Path path, long startEpoch) {
+        if (CACHE_EPOCH.get() != startEpoch) return false;
+        READY.put(url, path);
+        if (CACHE_EPOCH.get() != startEpoch) {
+            READY.remove(url, path);
+            try { Files.deleteIfExists(path); } catch (Throwable ignored) {}
+            return false;
+        }
+        return true;
+    }
+
     private static void download(String url, long startEpoch) {
         Path cacheDir = cacheDir();
         Path partPath = null;
@@ -259,18 +286,7 @@ public final class VideoCache {
             // Resume-friendly: if the file's already on disk from an earlier session, just
             // index it without re-downloading.
             if (Files.exists(finalPath) && Files.size(finalPath) > 0) {
-                // Pre-publish check — bail if clearAll has run since submit.
+                if (!publishIfNotCancelled(url, finalPath, startEpoch)) {
-                if (CACHE_EPOCH.get() != startEpoch) return;
-                READY.put(url, finalPath);
-                // Post-publish re-check: same window as the post-move check on the download
-                // path. If clearAll landed between the epoch read above and the READY.put,
-                // it would have wiped READY + the file, and our put() resurrected a stale
-                // entry pointing at a now-deleted path. Roll it back: remove our entry
-                // (compareAndRemove via remove(key, value) to avoid clobbering a concurrent
-                // legitimate re-put) and best-effort delete the file.
-                if (CACHE_EPOCH.get() != startEpoch) {
-                    READY.remove(url, finalPath);
-                    try { Files.deleteIfExists(finalPath); } catch (Throwable ignored) {}
                     VideoPlayerMod.LOG.info("[{}] preload: reindex cancelled (clearAll ran) — {}",
                         VideoPlayerMod.MOD_ID, url);
                     return;
@@ -370,21 +386,16 @@ public final class VideoCache {
             Files.move(partPath, finalPath, StandardCopyOption.REPLACE_EXISTING,
                 StandardCopyOption.ATOMIC_MOVE);
 
-            // Post-move cancellation check. clearAll() may have run between the pre-move
+            // Race-safe publish: this covers the window between Files.move and READY.put
-            // check above and the move itself — in that case clearAll's directory scan
+            // (clearAll deletes the freshly-moved file then we resurrect it in READY) AND
-            // either missed our file (it didn't exist yet) or saw the .part and skipped /
+            // the window between READY.put and the "완료" notification (clearAll wipes
-            // failed to delete it. Either way, finalPath now exists on disk but the
+            // READY then we re-publish + emit stale completion chat). publishIfNotCancelled
-            // user-visible cache state is "cleared", so we must delete the file and skip
+            // rolls back both READY and the on-disk file if the epoch moved at any time.
-            // both the READY.put and the "완료" chat. Without this check, a clear right at
+            if (!publishIfNotCancelled(url, finalPath, startEpoch)) {
-            // this window leaves a resurrected file in READY and a stale "완료" message.
+                VideoPlayerMod.LOG.info("[{}] preload: cancelled at publish (clearAll ran) — {}",
-            if (CACHE_EPOCH.get() != startEpoch) {
-                VideoPlayerMod.LOG.info("[{}] preload: cancelled after move (clearAll ran) — {}",
                     VideoPlayerMod.MOD_ID, url);
-                try { Files.deleteIfExists(finalPath); } catch (Throwable ignored) {}
                 return;
             }
-
-            READY.put(url, finalPath);
             VideoPlayerMod.LOG.info("[{}] preload: cached {} ({} bytes) -> {}",
                 VideoPlayerMod.MOD_ID, url, total, finalPath.getFileName());
             long mb = Math.max(1, total / (1024 * 1024));

src/main/java/com/ejclaw/videoplayer/client/render/VideoAnchorRenderer.java

@@ -31,8 +31,19 @@ import org.joml.Matrix4f;
 @Environment(EnvType.CLIENT)
 public class VideoAnchorRenderer implements BlockEntityRenderer<VideoAnchorBlockEntity, VideoAnchorRenderer.State> {
 
-    /** Tiny outward offset so the quad doesn't z-fight with the wall. */
+    /**
-    private static final float SURFACE_EPSILON = 0.001F;
+     * Outward offset so the quad doesn't z-fight with the wall it sits on.
+     *
+     * <p>Depth-buffer precision drops with the square of view distance: with near=0.05 and
+     * 24-bit depth, the smallest resolvable step at 30 blocks is ~1mm, and ~12mm at 100
+     * blocks. The old 0.001 (1mm) offset was right at the precision boundary at ~30 blocks
+     * — which is exactly the distance users started seeing the wall texture flicker through
+     * the video panel once {@code render_distance_blocks} was raised past the default.
+     *
+     * <p>2cm gives ~20× margin at 30 blocks and is still visually unnoticeable up close
+     * (~3% of a block thickness).
+     */
+    private static final float SURFACE_EPSILON = 0.02F;
 
     public VideoAnchorRenderer(BlockEntityRendererProvider.Context ctx) {
         // no-op
@@ -75,7 +86,12 @@ public class VideoAnchorRenderer implements BlockEntityRenderer<VideoAnchorBlock
         pose.translate(-0.5F, -0.5F, -0.5F + SURFACE_EPSILON);
 
         final Matrix4f mat = new Matrix4f(pose.last().pose());
-        RenderType rt = RenderTypes.entityCutout(tex);
+        // entitySolid (not entityCutout): video frames come from swscale → AV_PIX_FMT_RGBA with
+        // alpha hard-set to 255, so there is no alpha-tested cutout. Cutout's alpha-discard step
+        // adds nothing here and makes distant sampling unstable — without mipmaps on a dynamic
+        // texture, neighbouring texels can shimmer above/below the discard threshold at sub-pixel
+        // sampling rates, contributing to the flicker users see once render distance is raised.
+        RenderType rt = RenderTypes.entitySolid(tex);
         collector.submitCustomGeometry(pose, rt, (poseUnused, vc) -> {
             // Single-sided: the back of the anchor is by design pressed against the wall the
             // player clicked, so a back face is pure GPU waste. Halves the fragment shader work

src/main/java/com/ejclaw/videoplayer/command/VideoPlaceCommand.java

@@ -22,10 +22,30 @@ import net.minecraft.server.level.ServerPlayer;
 import net.minecraft.server.permissions.Permissions;
 import net.minecraft.world.level.block.Block;
 
-/** SPEC §4.5.1 — {@code /videoPlace <pos> <facing> <width> <height> <url>} */
+/**
+ * SPEC §4.5.1 — {@code /videoPlace} has two accepted forms:
+ *
+ * <ul>
+ *   <li><b>Legacy 5-arg</b>: {@code /videoPlace <pos> <facing> <width> <height> <url>}
+ *       — preserved for existing command blocks. Volume defaults to 50% and muted=false.
+ *       The {@code url} here is a single-token string so it doesn't swallow the {@code volume}
+ *       slot of the new form.</li>
+ *   <li><b>New 6-arg</b>: {@code /videoPlace <pos> <facing> <width> <height> <volume> <url>}
+ *       — {@code volume} is 0..100 (percent) or {@code -1} to start muted. The percent form
+ *       mirrors the GUI slider; {@code -1} is a CLI shortcut so admins don't need a follow-up
+ *       {@code /videoMute} step. {@code url} is greedy here, so URLs containing spaces (rare
+ *       but possible after URL-decoding) still work.</li>
+ * </ul>
+ *
+ * <p>Brigadier disambiguates the two by the type of the 5th argument: integer → new form,
+ * non-integer token → legacy form.
+ */
 public final class VideoPlaceCommand {
     private VideoPlaceCommand() {}
 
+    /** Default volume (percent) applied to the legacy 5-arg form. */
+    private static final int DEFAULT_VOLUME_PCT = 50;
+
     public static void register(CommandDispatcher<CommandSourceStack> dispatcher) {
         dispatcher.register(build("videoPlace"));
     }
@@ -38,12 +58,33 @@ public final class VideoPlaceCommand {
                 .then(Commands.argument("facing", StringArgumentType.word())
                     .then(Commands.argument("width", IntegerArgumentType.integer(1, 32))
                         .then(Commands.argument("height", IntegerArgumentType.integer(1, 32))
-                            .then(Commands.argument("url", StringArgumentType.greedyString())
+                            // New form: volume (int) + greedy url
-                                .executes(VideoPlaceCommand::run))))));
+                            .then(Commands.argument("volume", IntegerArgumentType.integer(-1, 100))
+                                .then(Commands.argument("url", StringArgumentType.greedyString())
+                                    .executes(ctx -> runNew(ctx))))
+                            // Legacy form: single-token url, no volume slot. Single-token string
+                            // is intentional so "<int> https://..." cannot be parsed as a legacy
+                            // url that happens to start with a number — Brigadier first tries the
+                            // new branch and only falls through here if "volume" isn't an int.
+                            .then(Commands.argument("url", StringArgumentType.string())
+                                .executes(ctx -> runLegacy(ctx)))))));
     }
 
-    private static int run(com.mojang.brigadier.context.CommandContext<CommandSourceStack> ctx)
+    private static int runNew(com.mojang.brigadier.context.CommandContext<CommandSourceStack> ctx)
             throws CommandSyntaxException {
+        int volumeArg = IntegerArgumentType.getInteger(ctx, "volume");
+        String url = StringArgumentType.getString(ctx, "url");
+        return runWithValues(ctx, volumeArg, url);
+    }
+
+    private static int runLegacy(com.mojang.brigadier.context.CommandContext<CommandSourceStack> ctx)
+            throws CommandSyntaxException {
+        String url = StringArgumentType.getString(ctx, "url");
+        return runWithValues(ctx, DEFAULT_VOLUME_PCT, url);
+    }
+
+    private static int runWithValues(com.mojang.brigadier.context.CommandContext<CommandSourceStack> ctx,
+                                     int volumeArg, String rawUrl) throws CommandSyntaxException {
         CommandSourceStack src = ctx.getSource();
         ServerLevel level = src.getLevel();
         BlockPos pos = BlockPosArgument.getLoadedBlockPos(ctx, "pos");
@@ -54,7 +95,12 @@ public final class VideoPlaceCommand {
         }
         int width  = IntegerArgumentType.getInteger(ctx, "width");
         int height = IntegerArgumentType.getInteger(ctx, "height");
-        String raw = StringArgumentType.getString(ctx, "url").trim();
+        // -1 is the CLI mute shortcut; the BE keeps the underlying volume so an admin can
+        // /videoMute false later without re-typing a level. Anything 0..100 sets %-volume and
+        // clears mute.
+        boolean placeMuted = volumeArg < 0;
+        float placeVolume = placeMuted ? 0.5F : (volumeArg / 100.0F);
+        String raw = rawUrl == null ? "" : rawUrl.trim();
         // Accept either an http(s) URL or a /videoCache add <name> entry: resolveUrlOrName()
         // returns the canonical URL in both cases, or null when a non-URL string didn't match
         // any named entry.
@@ -75,6 +121,8 @@ public final class VideoPlaceCommand {
         be.setWidth(width);
         be.setHeight(height);
         be.setUrl(url);
+        be.setVolume(placeVolume);
+        be.setMuted(placeMuted);
 
         CompoundTag nbt = be.toNbt();
         for (ServerPlayer p : PlayerLookup.tracking(level, pos)) {