search

wave-harmonic / crest

A class-leading water system implemented in Unity
MIT License
3.48k stars 480 forks source link

Waves not forming properly, LOD and other strangeness #279

Closed Draydak closed 5 years ago

Draydak commented 5 years ago

Describe the bug Fresh download of Crest (29th June 2019) from Git. Open in Unity, open any demo scene and the waves are all Muted / flat. The Whirlpool scene was craziest, see video. There does not appear to be any wake / bow wake on any boats. But it could just be so shallow, it's not visible.

LOD appears to be behaving oddly, also only appearing in bottom image slot (again see video)

Screenshots / video Crest Issue

image

Log No error's / problems in Log at all.

Versions Git version, master branch downloaded (29th June). Unity 2018.4.2f1

To Reproduce Download from Git master branch to empty folder. Open in Unity, open any demo scene. No changes, no modifications, not added to existing project etc.

Platform Editor and standalone Windows 10 Nvidia Geforce GT 650M

Additional context I believe the graphics card is fully DX11 compatible, so should be capable of handling required shaders. (it worked with earlier versions). All drivers are up-to-date.

moosichu commented 5 years ago

Hi @Draydak,

A few things which could help us debug this issue:

Doing the first one should be fairly easy, so do that first. If you can take the time to do this it would really help because we are unable to recreate the bug on our end! If any of the intructions are unclear, just let me know.

moosichu commented 5 years ago

Oh, another quick thing that you could do is a take a screenshot of what things look like when you uncheck Shape combine pass in the overlay on the left.

Draydak commented 5 years ago

image

Draydak commented 5 years ago

Unchecked shape combine pass, this made big difference image

moosichu commented 5 years ago

Awesome, the renderdoc capture would really help here (with shape combine pass activated so that bug itself is captured). I know which shader the bug is happening in, because I was able to replicate it on Linux, but a DX11 capture with debug symbols would really help as well.

Draydak commented 5 years ago

I need to download Renderdoc etc. (not something I've used) But work is dragging me a way for a bit so might not get that capture to you till later

moosichu commented 5 years ago

No worries, will let you know if I find the culprit beforehand

Draydak commented 5 years ago

Had a quick go, not sure if this is right as just followed my instincts.

CrestIssue279.zip

If you need me to toggle something more in RenderDoc / Unity that I've missed let me know

moosichu commented 5 years ago

Thanks. The capture itself looks fine, but it seems like you paused the game window before you made it which means that it doesn't contain a capture of the bugged behaviour. If possible please do the same again but whilst the game is running.

(If this isn't what happened, let me know and I will see if I can work out why the relevant info is missing from the capture).

moosichu commented 5 years ago

Another option could be to update your drivers (but send through capture first).

moosichu commented 5 years ago

The Linux issue appeared to be something else, as it went away after I restarted my PC. :/

Draydak commented 5 years ago

CrestIssue279.zip

Second attempt, triggered from inside Unity (aka read manual page more). At a glance some crest Lod data in there, but it's all voodoo to me. I'm not familiar with RenderDoc so feel free to spell out any specifics I need to do.

moosichu commented 5 years ago

That renderdoc capture is exactly what we were after and has already revealed some useful info.

I'm going to dig through it some more, but for some reason renderdoc is reporting that none of the animated wave shaders ever get dispatched (except for the one you are seeing). That would certainly explain the problem. Normally you get this if the kernel cannot be found or something similar, but that would spam errors to the log. I'm going to do some thinking and see if there is something we can do on our end to verify things.

moosichu commented 5 years ago

https://drive.google.com/file/d/1A8A76u96RJDnUo5Yuim6Wm3fk3630Qns/view?usp=sharing

Can you give this a go and tell me what results you get?

Draydak commented 5 years ago

image

Still seems to have same issue, the few errors mainly relate to PostProcessing from looks.

`Initialize engine version: 2018.4.2f1 (d6fb3630ea75) GfxDevice: creating device client; threaded=1 Direct3D: Version: Direct3D 11.0 [level 11.0] Renderer: NVIDIA GeForce GT 650M (ID=0xfd1) Vendor:
VRAM: 2007 MB Driver: 25.21.14.2531

Initializing input. Input initialized. Initialized touch support. The referenced script (UnityEngine.Rendering.PostProcessing.PostProcessResources) on this Behaviour is missing! (Filename: C:\buildslave\unity\build\Runtime/Scripting/ManagedReference/SerializableManagedRef.cpp Line: 195) The referenced script on this Behaviour (Game Object '') is missing! (Filename: C:\buildslave\unity\build\Runtime/Mono/ManagedMonoBehaviourRef.cpp Line: 294) A scripted object (probably UnityEngine.Rendering.PostProcessing.PostProcessResources?) has a different serialization layout when loading. (Read 52 bytes but expected 1356 bytes) Did you #ifdef UNITY_EDITOR a section of your serialized properties in any of your scripts? 0x00007FFFC926400C (UnityPlayer) 0x00007FFFC9267339 (UnityPlayer) 0x00007FFFC924DEBC (UnityPlayer) 0x00007FFFC9F73EC7 (UnityPlayer) UnityMain 0x00007FFFC9AE26AA (UnityPlayer) UnityMain 0x00007FFFC9AE4550 (UnityPlayer) UnityMain 0x00007FFFC9AE3C55 (UnityPlayer) UnityMain 0x00007FFFC9AE12FB (UnityPlayer) UnityMain 0x00007FFFC982B9BA (UnityPlayer) UnityMain 0x00007FFFC982C5B3 (UnityPlayer) UnityMain 0x00007FFFC982C9AD (UnityPlayer) UnityMain 0x00007FFFC98E59EF (UnityPlayer) UnityMain 0x00007FF83D907974 (KERNEL32) BaseThreadInitThunk 0x00007FF8403DA271 (ntdll) RtlUserThreadStart (Filename: C:\buildslave\unity\build\Runtime/Serialize/SerializedFile.cpp Line: 2001) The referenced script (UnityEngine.Rendering.PostProcessing.PostProcessLayer) on this Behaviour is missing! (Filename: C:\buildslave\unity\build\Runtime/Scripting/ManagedReference/SerializableManagedRef.cpp Line: 195) The referenced script on this Behaviour (Game Object 'Main Camera') is missing! (Filename: C:\buildslave\unity\build\Runtime/Mono/ManagedMonoBehaviourRef.cpp Line: 294) A scripted object (probably UnityEngine.Rendering.PostProcessing.PostProcessLayer?) has a different serialization layout when loading. (Read 32 bytes but expected 188 bytes) Did you #ifdef UNITY_EDITOR a section of your serialized properties in any of your scripts? 0x00007FFFC926400C (UnityPlayer) 0x00007FFFC9267339 (UnityPlayer) 0x00007FFFC924DEBC (UnityPlayer) 0x00007FFFC9F73EC7 (UnityPlayer) UnityMain 0x00007FFFC9AE26AA (UnityPlayer) UnityMain 0x00007FFFC9AE4550 (UnityPlayer) UnityMain 0x00007FFFC9AE3C55 (UnityPlayer) UnityMain 0x00007FFFC9AE12FB (UnityPlayer) UnityMain 0x00007FFFC982BA94 (UnityPlayer) UnityMain 0x00007FFFC982C5B3 (UnityPlayer) UnityMain 0x00007FFFC982C9AD (UnityPlayer) UnityMain 0x00007FFFC98E59EF (UnityPlayer) UnityMain 0x00007FF83D907974 (KERNEL32) BaseThreadInitThunk 0x00007FF8403DA271 (ntdll) RtlUserThreadStart (Filename: C:\buildslave\unity\build\Runtime/Serialize/SerializedFile.cpp Line: 2001) UnloadTime: 7.451700 ms`
huwb commented 5 years ago

Likely connected to #301

huwb commented 5 years ago

@Draydak we now have 3 cases of this bug occurring for users.

This is probably a long shot but i noticed above you said it was working for you on earlier versions. You don't happen to know a version/date that was working? It might allow us to compare the versions and give us leads..

Draydak commented 5 years ago

Version 7.0.0 definitely works ok for me, downloaded just other day as water is now under the microscope in my project.

I believe it was good until April / May time, but my Git skills are not good enough to confirm.

huwb commented 5 years ago

@Draydak if you have time could you please run this build, and attach a screenshot of what you get? This is taken just before we did a major upgrade to the core crest update - added compute shaders.

https://drive.google.com/open?id=1-UgFF60DSGRyqa5T3U3IYLb1vl4O-OIa

huwb commented 5 years ago

UPDATE: this failed to fix the error for one of the other people experiencing the issue, so it may be a failed fix.

@Draydak after much headscratching we may have ID'd the issue. could you please give this new build a test when you get a chance and let us know if it resolves the issue? (if it does, disregard the previous request above)

https://drive.google.com/open?id=1OBC2MXWC2KerxkKtujekMctnvHsUVSj3

thanks!

Draydak commented 5 years ago

Last one first "the fix" image Still no good I'm afraid

Draydak commented 5 years ago

The Crest-28ebf08b image

This does work, if that is any help to you.

moosichu commented 5 years ago

Thanks a lot for your patience with this. How does this build fair for you? This is a build made from #316, shadows and foam are broken but waves do work.

https://drive.google.com/file/d/19VYoMT43OQ97ZLrDnkIZBYQkNux4WsaV/view?usp=sharing

moosichu commented 5 years ago

Here's another build, it's deliberately broken, but you should get results like this:

image

https://drive.google.com/open?id=1YsS5eRyKZxOWKpSk2NIxak8XlMSYrc4B

Just trying to throw different things at the wall to see what sticks.

Draydak commented 5 years ago

the build from 316 looks as follows, sadly not working

image

Draydak commented 5 years ago

the simple array copy build:

image

Edit: When windows pastes the correct image

moosichu commented 5 years ago

Two builds incoming, they should both look like this (foam, etc. will be broken, but waves should work):

image

One of these builds has reverted back to using the pixel shader (which is what already works for you), so if that doesn't work we are then in interesting territory.

Build 1

https://drive.google.com/open?id=1szwYoCRGxEV4CSqKWMNIALYvbBMFG1R5

Build 2

https://drive.google.com/open?id=1-gu2QictZuxD4fWwJbajSzPcKJ3_Sz6l

Let me know how both of these go if possible, once again, thank you so much for testing!

Draydak commented 5 years ago

Build 1: image

Build 2: image

They both work! Hope that is a good thing

huwb commented 5 years ago

We just realised today that we think the three cases of this that we're aware of are likely all from unity 2018.

We should probably eliminate this important factor from the equation.. @Draydak if you get a chance could you please try this build i made from 2019? Thanks.. https://drive.google.com/open?id=1tKI0jlGswdKQadeyHTLjrn3XxlJD608s

EDIT nevermind, we just found out one of the other people experiencing this is on Unity 2019.2.0f1 , so that likely kills that theory..

moosichu commented 5 years ago

I've hijacked the issue description to summarise our findings. The issue does seem to be due to an attempt to read a RWTexture2D (which is weird, because writing to them is fine). That's assuming its not a bug with Unity 2018, hence Huw's test build above. I'm going to setup a bunch of experiments next week in a single Unity (non-crest) project to see if we can pin-down a common denominator. Thanks so much for your patience with this @Draydak!

Draydak commented 5 years ago

I've been trying on 2019 myself recently since I've moved my project to that in anticipation of changing it to LWRP. I tested the build anyway, but as expected does not work.

Although I'm doubtful that I will be able to use Crest in my current project and to be honest Crest is probably overkill for my current requirements. I cannot deny the performance of Crest, so it would be a shame not to use it. I can only afford to treat my project as a hobby at moment, so it moves at it's own pace.

I will try and support you guys in any way I can, it only takes a moment to test a build so it's the least I can do.

huwb commented 5 years ago

Technical findings so far

Summary from @moosichu: We still haven't pinned down the cause of the bug, but we have tried various experiments to verify what does and does not cause it.

It's worth noting that it only affects the ShapeCombine shader.

Here are a variety of configuration descriptions, alongside wether or not they presented the bug:

(🐛 means this configuration presented the bug, 🆗 means that bug wasn't present 😄 )

It's worth noting that in the renderdoc caputure, the shaders which were meant to dispatch don't show up as having dispatched at all. https://github.com/huwb/crest-oceanrender/files/3342958/CrestIssue279.zip

Related issues

301, #332

Hardware

GPU: GTX 1080, HD Graphics 4000, GT 650 M, GTX 750, Adreno 540 GPU, Intel HD Graphics 5500 CPU: 2X Intel i7 6800 k, i5-3320M, ?, ?, Snapdragon 835 chipset, ? RAM: 64gb, 4gb, ?, 8gb, ?, ? Windows: 8.1, 10 Pro, 10, 7, Android, ?

Spread of unity versions (both 2018 and 2019)

huwb commented 5 years ago

We have a new theory - it may be due to limited/patchy support for typed UAV loads. https://docs.microsoft.com/en-us/windows/win32/direct3d12/typed-unordered-access-view-loads

This build loads the wave data as a single Float32. it will look broken but waves should be visible. could i please get a test run of this with a screenshot of the result?

https://drive.google.com/open?id=1w-aOfXhubgM1aiuG_qfTN_mVcBVzq4SJ

As a side note, it may be worth trying other APIs like Vulkan for example (can be configured in the player settings), although this may be hardware limitation in the end. And this suggestion won't help the android devices.

Draydak commented 5 years ago

There's something on the LOD image

huwb commented 5 years ago

Yep thats what i get too, so that seems to confirm the issue.

I'll discuss how we might work around this with @moosichu . Thanks @Draydak .

wes-hawkins commented 5 years ago

Count me in on having this issue. I first imported Crest into a project on my work computer (with a GTX 1080), and the water looked perfect. I pushed the project to GitHub and headed home. When I got home I loaded it onto my home computer, which has a GTX 950 as well as a GTX 770, and was surprised to find the water was completely flat. Tried fiddling with all kinds of stuff to make sure the Gerstner shape generator was being picked up by Crest... wasn't throwing any errors, it just refuses to make waves.

Should note that I'm using the 'Digital Ruby' branch of WM, though he says he's made only 'very minor changes' in the branch that shouldn't have anything to do with this issue. I have tried pulling the up-to-date master branch of Crest and had the same results.

waves_no_work

    Version:  Direct3D 11.0 [level 11.0]
    Renderer: NVIDIA GeForce GTX 770 (ID=0x1184)
    Vendor:   
    VRAM:     4052 MB
    Driver:   23.21.13.9077
huwb commented 5 years ago

@moosichu came up with a scheme where the displacements is a single channel texture array, with contiguous sections of the array assigned to each channel:

[ x0 x1 x2 ... y0 y1 y2 ... z0 z1 z2 ... sss0 sss1 sss2 ...]

And we found a way to integrate this without too much hacking or special cases.

This build should produce waves - could we please get verifications that it works for you guys?

https://drive.google.com/open?id=12LxuKlEdZFPC6NPKnJ-l2LhMe3iwbMip

So that's the good news... the less good news is that it the displacement texture readback to CPU (for collision/buoyancy) is going to be a nightmare if we're needing to transfer each channel individually, it will make a complex system even more complex, and will double the required bandwidth.

@moosichu and I will discuss. We have plans to change up how the collision buoyancy is done and not transfer the entire displacement textures from the GPU, and this future implementation should work with this workaround, so that seems to be the best option. We don't have an ETA for this but hope to start experimenting with it soon.

Before the new system arrives, displacement texture readback will not work with these workarounds, and the other options are the Null Collider which is just a flat horizontal plane, or CPU Gerstner which will evaluate the waves on the CPU. The CPU collision does not support shallow water, so is only a good fit for deep, open ocean. There is a prototype which does take the depth caches into account, it's not been cleaned up or extensively tested but might be a good option: #268

Draydak commented 5 years ago

image

I see waves, sea waves... it appears functional! for me at least, hopefully others too.

It's a shame you can't just sample an area around the object you want buoyant, instead of feeding back all the data. But hopefully you clever chaps can come up with something workable.

moosichu commented 5 years ago

Absolute aces! Thank you for your patience and testing! 👍 This was definetely a weird issue, now that we know the solution will work, we can get this robust and polished.

wes-hawkins commented 5 years ago

image

Waves ahoy here as well! Very cool!

huwb commented 5 years ago

After discussing we werent happy with the drawbacks of the above, so time for yet another pivot - we made a new version which works around the UAV issue by using normal shaders instead of compute shaders. This adds a few passes but they are simple copies and it should not hit performance. On the plus side this doesnt suffer from the above limitations and 1 or 2 more, and keeps the solution nice and simple.

This should work but could i please get a confirmation or two, again. It looks like we're on the home straight now.

https://drive.google.com/open?id=1Hmnmyv2P1EMvUkurQIyx05jbxasGYb84

Thanks again for your patience..

Draydak commented 5 years ago

image

Can confirm, waves are working

huwb commented 5 years ago

Today a miracle happened - we merged in the above fix into the master branch. Could I please get verifications that it works on the latest version? Thanks!

huwb commented 5 years ago

Report of solutions (for the record):

Scrapping texture arrays

+Tested +Potentially more flexibility with dyn wave sim adaptive time steps -Code becomes more complex and ugly -More draw calls

More tradeoffs here: #316

Splitting into single channel per slice

-Changes semantic meaning of slices in this special case - channels multiplexed into slices, for displacements only -Require special case functions for sampling displacements -"Wrap" addressing doesnt work in depth dimension - uv (0, 0, 6.5) might lerp completely different channels.. -We could not get packing to work (branch fix/uav-support-2-pack-f16s), so this doubles memory/bandwidth requirement for the displacement texture array -Breaks displacement readbacks, although we want to overhaul this system anyway

Ping pong wave combine (implemented)

+Branches the combine code, rest of code/shaders/semantics remain unchanged -One extra texture copy per LOD to do the 'pong'

Draydak commented 5 years ago

Can confirm it is working! 😄

Observations Performance wise it does seem to have suffered, however if that's specific to this change or how much Crest has matured in last few months... I cannot say

On a positive note, the boat scenes are much more stable than I remember! Although I did turn around at one point and see a boat falling from the sky, so it's not perfect.

I'd be interested to hear other peoples experiences, especially on how or if it's affected performance.

moosichu commented 5 years ago

Compared to older versions of Crest, the example scenes have quadruple the resolution and more gerstner waves, which is probably where that hit comes from. Shouldn't affect projects with existing settings though.

huwb commented 5 years ago

Yeah that's right. The lod resolution may be a bit higher, and the dynamic wave sim does 1 or 2 more steps.

The new ping pong combine pass can be turned on/off at runtime using the debug gui which can be used to compare perf.

On Wed, 18 Sep 2019, 19:14 Tom Maenan Read Cutting, < notifications@github.com> wrote:

Compared to older versions of Crest, the example scenes have quadruple the resolution and more gerstner waves, which is probably where that hit comes from. Shouldn't affect projects with existing settings though.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/crest-ocean/crest/issues/279?email_source=notifications&email_token=AAHFO7JI66DJW5MBJCBAVA3QKJOXJA5CNFSM4H4KT2ZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7AZHZQ#issuecomment-532780006, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHFO7P2S6JBNR5ATHQF63DQKJOXJANCNFSM4H4KT2ZA .

huwb commented 5 years ago

Closing!

huwb commented 5 years ago

A new LWRP package has been submitted with this fix. It should go up in the next couple of days, I'll confirm when it does.

huwb commented 5 years ago

The fix is now live in version 2.2 of the asset.