patriciogonzalezvivo / vera

C++/WASM GL Framework
Other
82 stars 14 forks source link

fix REVERT_TO_PREVIOUS_SHADER by introducing previousSource variables #5

Closed themoep closed 1 year ago

themoep commented 1 year ago

The Problem: glslViewer is using setSource() and use() to render the canvas shader (and possibly others). The first overwrites the m_fragmentSource field and the latter calls load() with the (new) m_fragmentSource. However, load() assumes the passed source is new and reverts to the local m_fragmentSource if it fails compilation - which also fails since it is the same and always results in the SHOW_MAGENTA_SHADER behaviour.

To fix this, the successfully compiled shader sources are stored in the m_previousFragmentSource and m_previousVertexSource. In the worst case this doubles the memory required to save the shader source, but it should be negligible and can be disabled by setting the error handling to DONT_KEEP_SHADER.

Tested with the latest glslViewer https://github.com/patriciogonzalezvivo/glslViewer/commit/b17ee73091a3890ecc152fc85c8c2bdaeba92483 - don't forget to set error_screen,off when testing like I did :sweat_smile:

themoep commented 1 year ago

one more thing: I opted to add the fields because it makes the behavior coherent across different uses of the class, and does not require any changes to the API or glslViewer.

patriciogonzalezvivo commented 1 year ago

Hi @themoep! thanks for bringing this up. I agree the current system is a bit over convoluted and had some state issues. For context lot of the code mutate for bringing a sync compatibility so users can drag and drop shaders on glslViewer or loading code on the wasm version of it, and prevent it to compile outside the main GL thread. Getting this iron out would be amazing! thanks in advance for putting the effort!

themoep commented 1 year ago

I should be able to do some more tests on the weekend as I was just looking at glslViewer's 2D shaders, but it works there now :) and thank you for such a great tool :D

patriciogonzalezvivo commented 1 year ago

Awesome! Thank you! Let me know when I can try it on my side. The current committed change only adds the variables on the header file, I think maybe you miss staging the changes on the .cpp file

themoep commented 1 year ago

whoops, you are right :facepalm: I did already change the cpp file but didn't push it.

themoep commented 1 year ago

found a bug in the proposed patch that saving a shader twice that does not compile triggers the magenta screen still (I'd expect it to keep the last working shader), so this pull request is not ready. I'll look into it a bit more, but will move this to an issue until I have a better pull request :)

Edit: opened issue #6

patriciogonzalezvivo commented 1 year ago

Thanks for going down this rabbit hole, looking forward for your findings