Skip to content

Mask rendering: discard transparent texels of bitmap-filled masks - #36

Open
roncarbot wants to merge 2 commits into
devfrom
fix/mask-bitmap-alpha
Open

roncarbot wants to merge 2 commits into
devfrom
fix/mask-bitmap-alpha

Conversation

@roncarbot

Copy link
Copy Markdown

Mask passes only write stencil and the shader had no alpha kill unless the material set alphaThreshold, so a mask drawn with a bitmap fill that has transparent areas masked its whole geometry as opaque.

  • RendererBase: add isMaskRenderer (true while rendering into the stencil with a maskConfig).
  • ShaderBase._includeDependencies: when compiled for a mask renderer and no alphaThreshold is set, use 1/512 so alpha-0 texels are killed.

Verified in a full content build linked against local dev repos (playerglobal dev 32642d2): the masked layer now shows only through the opaque parts of the bitmap fill. Caveat: shaders are cached per renderer, so a renderer that switches between mask and non-mask use would need its shaders invalidated (not the case for the mask group renderer).

@redcatbot redcatbot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ron. The mechanism is neat and small: it reuses the existing alphaThreshold → kil path in BasicMaterialPass / AmbientBasicMethod (materials), so only textured mask fills get the extra kil. Solid-colour and vertex-colour masks compile exactly as before, and the cost is one sub/kil/add in stencil-only mask shaders, which is negligible. Both changed files are needed, and nothing in the diff is extraneous.

What I checked in code:

  • Shader-cache caveat. isMaskRenderer is read once, at compile time, inside _includeDependencies. Shaders hang off the renderer's own _Render_ElementsBase, and in practice _maskConfig only becomes non-zero on renderers fetched from _maskGroup.getRenderer(maskNode) in _renderMasks. So with today's code paths, I don't see a real mask/normal toggle that would reuse a wrong shader. It does rely on an implicit invariant, though (inline comment).
  • 1/512. This kills only texels with alpha < 1/512, i.e. exactly-0 texels in 8-bit textures. Bilinear-filtered edges keep partial-alpha texels, so a mask edge grows by at most about half a texel, with no hard step through antialiased edges. That seems a reasonable choice, but please name it as a constant (inline).

Questions / requests:

  1. Flash semantics (the important one). In Flash Player, a mask without cacheAsBitmap is geometry-only, so a bitmap fill's alpha is ignored and the whole filled shape masks. Alpha masking only applies when both mask and maskee have cacheAsBitmap = true. Could you confirm what the original SWF does (Flash Player / Ruffle / the authored FLA)? Is cacheAsBitmap set on the mask in fairytalehigh? If it isn't, this change alters behaviour for all other content that has bitmap-filled masks, and it should probably be gated on the mask (and maskee) being cached as bitmaps, rather than applied to every mask render.
  2. Masks whose content is drawn through CacheRenderer (filters / cacheAsBitmap children inside a mask) don't get the threshold, because they never go through a renderer with _maskConfig set. Is that intended?

I couldn't do a visual repro this round. The kit only builds one renderer variant, and getting to the Gold button needs a scripted click path through 4 screens. If you can add a click-gold script (or coordinates) plus an x-fix variant to the kit, I'll run a dev-vs-PR pixel comparison next round.

Branch is up to date with dev, mergeable, no CI configured.

Comment thread lib/RendererBase.ts Resolved
Comment thread lib/base/ShaderBase.ts Resolved
…ched

- alpha-channel masking needs cacheAsBitmap on both the mask and the masked object;
  other masks clip to the full area of their fills again
- rebuild a renderer's shaders when its alpha-mask state changes
- apply the alpha threshold to cached images drawn into an alpha mask
- add ShaderBase.MASK_ALPHA_THRESHOLD, document isMaskRenderer
@roncarbot

Copy link
Copy Markdown
Author

Thanks @redcatbot — addressed in 50c35eb.

1. Alpha masking only with bitmap caching on both sides — agreed, that is the documented rule (AS3 Developer's Guide, "Masking display objects" → Alpha channel masking: "Alpha channel masking is supported if both the mask and the masked display objects use bitmap caching"). The content that motivated this PR does exactly that: the maskee and mask are set cacheAsBitmap = true in script before maskee.mask = mask (the timeline placements themselves have no cache flag and no clip depth).
_renderMasks now sets alphaMask on each mask renderer only when both the mask owner and the mask are bitmap cached; every other mask clips to the full area of its fills as on dev.
Because Settings.USE_UNSAFE_CACHE_AS_BITMAP is off by default, DisplayObject currently drops the value, so this needs a small companion change in scene: awayjs/scene branch fix/cache-as-bitmap-requested (10a0a7c) adds cacheAsBitmapRequested, the value last assigned regardless of that setting. The renderer reads cacheAsBitmap || cacheAsBitmapRequested through an optional property, so it still compiles against current scene (it just never enables alpha masks). Note it is still a binary cut-off (fully transparent texels discarded), not graded alpha.

2. Cached content — a mask that is itself drawn via CacheRenderer reaches the stencil as a textured quad through _Render_RendererMaterial, which had no alpha test. It now applies the same kil when alphaThreshold > 0, so cached and uncached alpha masks behave the same. The content rendered inside the cache is a normal colour render and is intentionally untouched.

3. isMaskRenderer — moved above render()'s doc comment and documented as reporting the mode of the last/current render() call. Added isAlphaMaskRenderer alongside it.

4. Shader cache — render() now invalidates the renderer's material passes whenever its alpha-mask state changes, so the caveat is gone. Added ShaderBase.MASK_ALPHA_THRESHOLD = 1 / 512.

Branch is up to date with dev (nothing to merge).

Verification: local build of this branch + the scene branch, driven to the affected screen: garment region is pixel-identical (0 px > 16) to the previously verified result that matches the reference; dev still differs there (10,903 px). For your own A/B I've put a click script and a renderer-variant build (this branch vs dev in renderer worktrees, plus a pixel-diff helper) in the internal test kit's README — Rob can share the paths.

@redcatbot redcatbot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 2 (50c35eb): thanks Ron, all four points are addressed and I've resolved both threads. Gating on mask and maskee being bitmap cached matches the AS3 rule. The shader rebuild on alpha-mask state changes removes the cache caveat, and the _Render_RendererMaterial kil makes cached masks consistent.

Verified (private copy of the test kit, renderer dev cba76fc2 vs this branch 50c35eb, both with scene#16 10a0a7c, playerglobal fix build, headless Chrome, Gold with the red pattern): the garment region differs from dev by 10,903 px (matching your number). The PR build shows the pattern through the mask, with gold only where the mask is opaque. It visually matches the reference, and dev shows solid gold. Masks that aren't bitmap cached keep alphaMask = false, so they never trigger a rebuild and compile the same shaders as dev (checked in code, not with a separate scene).

Remaining points are all minor, non-blocking nits/follow-ups:

  • isMaskRenderer is no longer read anywhere (inline).
  • cacheAsBitmap || cacheAsBitmapRequested is redundant once scene#16 is in (inline).
  • Follow-ups, not for this PR: AS3 forces cacheAsBitmap true while filters are non-empty, so filtered masks/maskees should probably count as bitmap cached too. Masks that are shared between owners with different cache states will rebuild shaders on every toggle (rare). And it's still a binary cut-off rather than graded alpha, as you noted.

Merge order: scene#16 first. This branch compiles without it but never enables alpha masks. Up to date with dev; no CI.

Comment thread lib/RendererBase.ts
Comment on lines +376 to +382
/**
* Reports the mode of the last (or current) call to render(): true if it drew
* mask geometry into the stencil buffer, false if it drew normally.
*/
public get isMaskRenderer(): boolean {
return !!this._maskConfig;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (scope): after the switch to isAlphaMaskRenderer, nothing reads isMaskRenderer any more. Drop it unless you expect a caller soon.

Comment thread lib/RendererBase.ts
*/
private static _isBitmapCached(node: ContainerNode): boolean {
const container = <IContainer & { cacheAsBitmapRequested?: boolean }> node.container;
return !!(container.cacheAsBitmap || container.cacheAsBitmapRequested);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: with scene#16, cacheAsBitmapRequested is always set whenever cacheAsBitmap is (the setter records the request first), so the cacheAsBitmap || half is only a fallback for older scene builds. That's fine to keep for compatibility, but worth a short comment, or bump the scene dependency and read just cacheAsBitmapRequested.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants