gemrb / gemrb

GemRB is a portable open-source implementation of Bioware’s Infinity Engine.
https://gemrb.org
GNU General Public License v2.0
929 stars 178 forks source link

Spell effect visible under walls #897

Open FrElvire opened 3 years ago

FrElvire commented 3 years ago

Bug description

In this battle with draconis (dragon form), you can't see draconis as you are too far from him but you can always see the spell effects, very convenient to launch zone spell without being too close but I suspect you shouldn't not see them.

Evidently this case is just an example.

GemRB version: kmfrick los-slim-check branch (SDL1 & SDL2), subviews (SDL2+OGL)

Expected behavior

If you don't see a character as he is too far, you shouldn't see spell effect either

Screenshots

spell_effect_far spell_effect_far_2

lynxlynxlynx commented 8 months ago

I could reproduce enough of this.

As part of trying to resolve it I've added more wallgroup occlusion, specifically to projectiles and scripted animations. It's not fully functional yet though. Besides what I noted for projectiles in 65a27542dc, the vvcs attached to the actor and blended standalone ones suffer from the same problem. I suspect it's the same reason we manage to appear to draw over the fog, even though fog gets rendered later.

@bradallred any ideas? I didn't manage to find out why BlitFlags::ONE_MINUS_DST is the problem, but interestingly making IE_VVC_BLENDED a noop (so the blit flag doesn't get set) actually didn't cause any obvious regressions — transparency still worked.

bradallred commented 8 months ago

IE_VVC_BLENDED is a misnomer on our part. These things are always blended, the flags only control the mode of blending. See #1291.

Basically, if the resource has a black background (check in NI), then use ONE_MINUS_DST (or possibly DST or SRC). Otherwise, just using BLENDED is correct.

If we are adding IE_VVC_BLENDED somewhere programmatically, it's probably not the right thing to do. I wouldn't be surprised if we could completely remove ScriptedAnimation::SetBlend, it doesn't seem like something that would be contextual.

As for the stencils there isn't anything in the game logic that I know of making choices based on these flags or the BlitFlags other than IE_VVC_NOCOVER. So first check that the appropriate bit in BlitFlags::STENCIL_MASK is set.

If all that checks out then it's probably something going on in OpenGL: https://github.com/gemrb/gemrb/blob/c141401e3f34d4831fdf8de27e6ee51ac9ffb381/gemrb/plugins/SDLVideo/Shaders/BlitRGBA.glsl#L30-L38

you can try disabling u_dither. I doubt it's the cause, but it makes troubleshooting stencilFactor easier. The only thing that occurs to me is that the texture may not have an alpha channel, but I'm assuming these are also coming from BAM resources so I dont think that should be broken.

bradallred commented 8 months ago

The other thing that occurs to me, is that the stencil possibly isn't large enough. see Actor::UpdateDrawingRegion. But if these effects aren't actually attached to an actor it's probably better to use the Map stencil.

lynxlynxlynx commented 8 months ago

The stencils appear fine, the debug rects really helped with nailing that down. Even just passing only BlitFlags::STENCIL_BLUE didn't change things, but yeah, ended up in the GL code as well. Will try your suggestions.

I think the black stuff is mostly in pst, so I'll check how some of the SetBlend removals affect things. However a lot of vvcs come with IE_VVC_BLENDED pre-set, so more work will be needed. We could perhaps just ignore it and use a bit of our own for the black cases. It will be clearer once we find them.

bradallred commented 8 months ago

I think the black stuff is mostly in pst, so I'll check how some of the SetBlend removals affect things.

There are definitely a lot of BG2 projectiles/effects that have the black background too. Magic missile is one that ought to apply everywhere.

However a lot of vvcs come with IE_VVC_BLENDED pre-set, so more work will be needed. We could perhaps just ignore it and use a bit of our own for the black cases. It will be clearer once we find them.

What are some examples where you feel like what is set in the data is not what we want? I wasn't suggesting we ignore the bit, I'm suggesting we probably should only pay attention to what is set in the data and our addition and use of ScriptedAnimation::SetBlend is probably a side-effect of our previous "solution" of using our own generated palettes instead of the appropriate blending mode. Now that we no longer do that I would think this manipulation is not necessary, and perhaps even causing issues.

ONE_MINUS_DST blending (or any other blending) shouldn't be affecting stenciling, so that would be an issue independent of whatever blending flags are set.

One thing of note, is that we aren't completely following the blending described by the EE developer here. Tho that also seems lacking as it doesn't describe what the combination of both flags ought to be. Is there just nothing (outside IWD) where both are set?

lynxlynxlynx commented 8 months ago

The two bits mentioned in the thread we haven't even implemented yet. IE_VVC_COPYFROMBACK and IE_VVC_3D_BLEND.

Re examples: anything I tried that had it set had wrong occlusion. Fireshield, the center of a fireball, comet are some.

lynxlynxlynx commented 8 months ago

Ah, wait, those are the projectile flags, not vvc: PTF_TRANS and PTF_BLEND

Shouldn't these sets be exclusive from what was posted? Now we can set several flags if more than one bit is on:

    if (TFlags & PTF_TRANS) {
        flags |= BlitFlags::ONE_MINUS_DST;
    }
    if (TFlags & PTF_BLEND) {
        flags |= BlitFlags::DST;
    }
    if (TFlags & PTF_TRANS_BLEND) {
        flags |= BlitFlags::SRC;
    }
bradallred commented 8 months ago

it doesn't matter if the bits are exclusive; there can only be one blend mode applied. Right now that means some blend mode has higher precedence, but I have no idea if we have chosen the correct one.

The post also indicates that the combination of both bits selects yet another blend mode (for IWD at least). It would be interesting to see what, if anything, has both bits set. Analyzing the behavior of those in the originals might be informative. It might be that when both bits are set we select blend mode of one or the other based on some context.

Re examples: anything I tried that had it set had wrong occlusion. Fireshield, the center of a fireball, comet are some.

right, but blend mode shouldn't be affecting this at all, so blending is a separate issue. Remove the flag from something that has it set and it should be noticeably drawn incorrectly.

lynxlynxlynx commented 8 months ago

Magic missile has both (bg2). That's the PTF_TRANS_BLEND case there. But it has proper precedence in the renderer.

A good example are the casting glows. They're bams, so us adding the blending bit in fx_casting_glow is needed (verified in bg1, bg2, pst). But at least in bg2, walls should cover them and they don't with that bit set.

bradallred commented 8 months ago

I imagine the bug is that in the fixed function OpenGL rendering, blending takes place after the fragment shader runs. The only fix would be to implement blending in the shader instead, which is more costly, but I don't know to what degree. The bigger annoyance is further fragmentation on the SDL backend.

I think my own effort is better spent on the backends outlined in #1771 which wouldn't have this issue, but if you have the drive and time it's pretty straight forward to implement blending in our glsl shader.

bradallred commented 8 months ago

one quick and dirty thing to try, might be to set all the channels of anything occluded to 0 in the shader. This of course would only work for total occlusion.

lynxlynxlynx commented 8 months ago

Disabling dithering in the shader didn't affect it, while setting the frag color to black cleared everything. Not that helpful, but sure, we can postpone this.

Dreamkins commented 6 months ago

If enable ONE_ONE_MINUS_DST flags |= BlitFlags::ONE_MINUS_DST; try disabling stancil flags &= ~BlitFlags::STENCIL_MASK; Helps with magic missile, casting glow and others