Repository navigation
Conversation
redcatbot
left a comment
There was a problem hiding this comment.
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.
isMaskRendereris read once, at compile time, inside_includeDependencies. Shaders hang off the renderer's own_Render_ElementsBase, and in practice_maskConfigonly 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:
- Flash semantics (the important one). In Flash Player, a mask without
cacheAsBitmapis 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 havecacheAsBitmap = true. Could you confirm what the original SWF does (Flash Player / Ruffle / the authored FLA)? IscacheAsBitmapset 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. - 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_maskConfigset. 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.
…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
|
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 2. Cached content — a mask that is itself drawn via 3. 4. Shader cache — 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
left a comment
There was a problem hiding this comment.
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:
isMaskRendereris no longer read anywhere (inline).cacheAsBitmap || cacheAsBitmapRequestedis redundant once scene#16 is in (inline).- Follow-ups, not for this PR: AS3 forces
cacheAsBitmaptrue whilefiltersare 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.
| /** | ||
| * 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; | ||
| } |
There was a problem hiding this comment.
Nit (scope): after the switch to isAlphaMaskRenderer, nothing reads isMaskRenderer any more. Drop it unless you expect a caller soon.
| */ | ||
| private static _isBitmapCached(node: ContainerNode): boolean { | ||
| const container = <IContainer & { cacheAsBitmapRequested?: boolean }> node.container; | ||
| return !!(container.cacheAsBitmap || container.cacheAsBitmapRequested); |
There was a problem hiding this comment.
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.
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.
isMaskRenderer(true while rendering into the stencil with a maskConfig).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).