Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CPUParticle2D Textures Don't Render on iOS After Screen Reading Shader #38211

Closed
Tracked by #38441
anthonyromano opened this issue Apr 26, 2020 · 14 comments
Closed
Tracked by #38441

Comments

@anthonyromano
Copy link

anthonyromano commented Apr 26, 2020

Godot version:
3.2.2 Beta 1

OS/device including version:
iOS, iPhone 11 Pro Max

Issue description:
If there's a shader with access to SCREEN_TEXTURE in the scene tree prior to a CPUParticle2D, the CPUParticle2D will not render its textures on iOS

Steps to reproduce:
Create a simple textured CPUParticle2D. Create a node with a simple shader that sets it's COLOR to the SCREEN_TEXTURE. Make sure the shaded node is before the CPUParticle2D in the scene tree. Build for iOS. Observe you can't see the particle.

It doesn't seem to matter if batching is on or off.

Minimal reproduction project:
Attached is such a project. Everything works fine, except on iOS. If you disabled the node with the shader, the particle will show up.
BrokenParticles.zip

@clayjohn
Copy link
Member

That may be from the batching PR. Could you try turning batching off in your project settings? It is under rendering/GLES2.

@anthonyromano
Copy link
Author

Batching on or off doesn't seem to make a difference.

@lawnjelly
Copy link
Member

lawnjelly commented Apr 26, 2020

You say 'previous builds', but just to confirm, does this work in 3.2.1, which builds have you tried, so we know where the regression is from? It's just possible it is something in the legacy path, I'm just double checking.

Edit - Have checked legacy path against the git history before batching. It is very slightly different from the original because it breaks _canvas_render_item into a function and passes the parameters in RenderItemState, but other than that I can't immediately see any differences (although this is checking by eye). So I'll wait for more info about which version this regression appeared in before looking further.

There was a bug with state.canvas_texscreen_used that has since been fixed (in the batching path), although that would have showed up on all platforms. This issue being iOS only suggests it is something different.

@anthonyromano anthonyromano changed the title Regression: CPUParticle2D Textures Don't Render on iOS After Screen Reading Shader CPUParticle2D Textures Don't Render on iOS After Screen Reading Shader Apr 26, 2020
@anthonyromano
Copy link
Author

@lawnjelly sorry about that! I thought for sure I had it working when I tested in 3.2.1 but I must have gotten myself mixed up and built with the shader disabled (I probably should have made the shader tint the screen in the sample project to make it more obvious :) )

Anyway, I've since edited the title and description.

@lawnjelly
Copy link
Member

lawnjelly commented Apr 26, 2020

That's alright 👍 , so now we can confirm it doesn't work in 3.2.1 or 3.2.2 beta. Have you seen it working in any previous versions (i.e. did it work in 3.2 stable, 3.1 etc)? Any info like this will be helpful and will enable us to get some ideas which commits might have affected it.

If you've not seen it working before, it is possible it has never worked on this hardware.

It is also quite easy to mess up with which export template you are using, I do this all the time with Android templates (the template must match the build you are using for the editor).

@lawnjelly
Copy link
Member

Actually thinking about it, I can see that the whole technique of screen reading could be very expensive / involved on tiled renderers (most mobile devices), or even not supported. You may find after performance testing that it might be best avoided.

This is just a guess how it may have to do it : it may have to do 2 passes for every layer that does a screen read

  1. Render every tile, then copy the tile results to the screen read texture.
  2. Finish every tile (because it cannot guarantee what UV will be read in the next stage)
  3. Restart rendering every tile again, after making a copy of the original tile, and then using the screen read texture as source.

The entire 2nd stage is dependent on the entire first stage finishing, and on top of that it must make a copy of the tiles between stages. I haven't looked in detail at how Godot does its screen reading.

Ideally to keep a tile renderer flowing at full speed you don't want any dependencies between the tiles like this.

@anthonyromano
Copy link
Author

anthonyromano commented Apr 26, 2020

So a couple things to clarify

This is ONLY an issue with textures on particles. If I keep them textureless, the particle renders fine. If I add a texture, it disappears (if the screen reading shader is before it in the scene tree)

As for performance, my primary target is iPhone and I've tested the game down to iPhone 6 and it renders at constant 60fps, so I don't think there's a concern there.

Here's the odd thing. In my actual game (which is a year old large project at this point with a lot going on), I have a textured particle effect that does not work with my "lights" (which use screen reading shaders) on 3.2.2 beta but DOES work on 3.2.2.rc (the version linked from the blog announcing GLES2 batching).

The problem is I can't figure out exactly how to recreate this in a simple project as it seems to be consistent behavior across both godot versions when I make one.

@lawnjelly
Copy link
Member

Here's the odd thing. In my actual game (which is a year old large project at this point with a lot going on), I have a textured particle effect that does not work with my "lights" (which use screen reading shaders) on 3.2.2 beta but DOES work on 3.2.2.rc (the version linked from the blog announcing GLES2 batching).

That could easily be due to #38137 or #38200, which have both been fixed. These seem unrelated to the issue mentioned here (I'm presuming the problems mentioned above also occur on desktop). The change causing those two bugs between the 3.2.2.rc and 3.2.2 beta is the batching across z indices commit, I don't think this was in either of the 3.2.2 batching test releases, but was present in the beta.

These were bugs that were due to logic in the batching, however this iPhone 11 issue appears quite different, if it occurs on 3.2.1 (before batching was introduced) and only occurs on certain hardware. It is usually better to stick to one bug per issue (unless we are confident they are duplicates).

It's probably frustrating because I can't make builds for you to test, you can build yourself the current 3.2 from source if you are confident to try, this will give you the most current version.

Just to clarify (and disambiguate from those other bugs), does the issue mentioned in this thread occur on other models of iPhone? Or just on iPhone 11 Pro Max?

@anthonyromano
Copy link
Author

It occurs on all iOS devices I tested (two iPhones and an iPad)

@lawnjelly
Copy link
Member

It occurs on all iOS devices I tested (two iPhones and an iPad)

Ah that is useful info. It may be related to tile rendering, I'll try and test on an Android phone if I get some time (just on the chance it occurs there too), this kind of thing will probably need someone familiar with the rendering to debug who has access to a device it occurs on.

There's also a few possibly related Android issues with SCREEN_TEXTURE e.g. #34318. Choosing frame buffer as '3D' instead of '3D without effects' worked there.

@anthonyromano
Copy link
Author

I had my framebuffer as "2D", I can test again with 3D

@clayjohn
Copy link
Member

clayjohn commented May 3, 2020

This appears to be the same as #38318, If so it is potentially fixed by #38318 (comment)

@lawnjelly
Copy link
Member

Just a note that this bug has an experimental fix available in the latest 3.2.2 beta. Would appreciate if any of you guys with the issue can test, full instructions are in #38441 (it's slightly more involved because of my incompetance! 😀 )

@akien-mga
Copy link
Member

No answer in a few months, so assuming that this is the same issue as #38318 and is thus fixed.

@akien-mga akien-mga added this to the 3.2 milestone Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants