MyGUI / mygui

Fast, flexible and simple GUI.
http://mygui.info/
Other
714 stars 205 forks source link

OgrePlatform - OpenGL: Sampler value sampler has not been set. #194

Closed sercero closed 3 years ago

sercero commented 3 years ago

The Fragment Program is not receiving the sampler value.

The GUI shows anyway but in OpenGL the log is being filled with the following message:

11:08:59: Error prior to using GLSL Program Object : GL_INVALID_VALUE
Validation warning! - Sampler value sampler has not been set.
Validation successful.
sercero commented 3 years ago

@Altren I have been looking around in MyGUI_OgreRenderManager.cpp searching for a place to set the sampler but it seems that no material is being assigned to the RTT, am I wrong?

In order to set the sampler there has to be a material pass, because otherwise OGRE says that there is no parameter named "sampler".

paroj commented 3 years ago

yeah, MyGUI does not really use Ogre, but rather tries hard not to use it. Thats why it is affected by some subtle low-level changes on the RenderSystem side. To fix this, you have to use the Ogre::Sampler objects. However I drafted a more future-proof approach in #196

sercero commented 3 years ago

Thanks a lot @paroj, there are many people in the OGRE community using MyGUI.

sercero commented 3 years ago

@paroj thanks for taking your time to look into this. I have tried your fork and found the following problems:

paroj commented 3 years ago

font issues should be solved with this: https://github.com/MyGUI/mygui/compare/541c27af2dc6a3c5e0f9bcf210e61362002c2c43..8cdc87632e2a56c83371abc2b868b93c465bf623

sercero commented 3 years ago

@paroj thanks again, I really appreciate it.

The white fonts in DirectX11 and OpenGL3+ are solved by that change when using OGRE 1.12.7.

But when using OGRE 1.12.9 the font does not show when using DirectX11 Renderer.

And there are still white models in the following demos: Demo_Gui, Demo_Pointers, Demo_RenderBox.

paroj commented 3 years ago

this should fix the white models: https://github.com/MyGUI/mygui/compare/8cdc87632e2a56c83371abc2b868b93c465bf623..0a7c41dc8c9f957b1bc319379aace15406793a21

which is a usage error on MyGUI side really, but you pushed me to fixing that as well..

paroj commented 3 years ago

looking at D3D11 actually led me to a bug in Ogre: https://github.com/OGRECave/ogre/pull/1771

however this can also be fixed in MyGUI: https://github.com/MyGUI/mygui/compare/0a7c41dc8c9f957b1bc319379aace15406793a21..45749923aec7523b513571097ba2ef7fc3bc49ee

Altren commented 3 years ago

@sercero If you are interested you can complete pull request, started by paroj. Main missing thing seems to be missing it custom shader suport, that is used in Msdf demo internally and probably not used externally yet.

sercero commented 3 years ago

@paroj you are on a roll. Thanks!

sercero commented 3 years ago

@Altren, I think that in order to have custom shader support then one needs to use the RTSS (Real Time Shader System) which @paroj integrated into the OgrePlatform.

It is my understaning that the proper way is to have the RTSS generate the shader, but I have never used that system.

Unfortunately I don't have the knowledge to do this for now.

sercero commented 3 years ago

@Altren now I understand how this works, when MSDF is being used then you change the Pixel Shader to one of these: Msdf_Ogre_FP.(glsl|glsles|hlsl).

I will see what I can do, but I cannot promise anything because my knowledge of shaders is extremely limited and I don't have much time either.

sercero commented 3 years ago

@Altren is there a way we can chat?

Is there a MyGUI forum or gitter/discord?

Because I want to understand some things I am seeing in MyGUI_OgreRenderManager.cpp.

I don't want to make big changes without having an understaning of the greater picture.

sercero commented 3 years ago

Hello @altren,

I have been thinking about how to modify the pull request by @paroj.

This is what I came up with:

With the changes made by @paroj this should be doable.

What do you think?

Altren commented 3 years ago

Is there a MyGUI forum or gitter/discord?

No. Lets keep it here for now.

Get rid of the RTSS dependency since there might be MyGUI+OGRE users who don't use RTSS and don't even have it enabled.

I completely agree with this, but what worries me is that it would require to add all shaders related code back.

Put the regular shaders into a material definition like "MyGUI/Default"

Agree, but keep shader definition in a separate files (as it is now), so users can modify shaders if necessary.

Put the Msdf shaders into another material definition like "MyGUI/Msdf"

Same as above, but keep in mind, that unlike "Default" materian this one would be defined in runtime, based on definition in code/xml. It might be ok to make MyGUI::OgreRenderManager::registerShader have material as argument instead of two shaders. The only issue I see is that currently this function overrides MyGUI::RenderManager::registerShader virtual function, but it might be ok to remove that virtual function. The only cos of that would be calling MyGUI::<platformName>RenderManager::getInstance().registerShader instead of MyGUI::RenderManager::getInstance().registerShader for each platform in the Demo_MsdfFont/DemoKeeper.cpp

Create a function setMaterial("MyGUI/Msdf") for OgrePlatform to change the material/shaders in use.

Ok. But keep in mind, that you will be forced to use same API for MyGUI::ITexture::setShader, that is used in other render systems (non-Ogre). Btw its ok to rename all this shaders related functions, since those were never released yet.

sercero commented 3 years ago

@Altren I thought it was OK to have a function named differently because of this code:

        // MsdfFontShader uses custom fragment program and default vertex program
#if defined(MYGUI_OGRE_PLATFORM)
        auto& renderManager = MyGUI::OgreRenderManager::getInstance();
        renderManager.registerShader("MsdfFontShader", "MyGUI_Ogre_VP." + renderManager.getShaderExtension(), "Msdf_Ogre_FP." + renderManager.getShaderExtension());
#elif defined(MYGUI_OPENGL_PLATFORM)
        // TODO not implemented in RenderManager
#elif defined(MYGUI_DIRECTX_PLATFORM)
        // TODO not implemented in RenderManager
#elif defined(MYGUI_DIRECTX11_PLATFORM)
        MyGUI::RenderManager::getInstance().registerShader("MsdfFontShader", "MyGUI_DirectX11_VP.hlsl", "Msdf_DirectX11_FP.hlsl");
#elif defined(MYGUI_OPENGL3_PLATFORM)
        MyGUI::RenderManager::getInstance().registerShader("MsdfFontShader", "MyGUI_OpenGL3_VP.glsl", "Msdf_OpenGL3_FP.glsl");
#elif defined(MYGUI_OPENGLES_PLATFORM)
        MyGUI::RenderManager::getInstance().registerShader("MsdfFontShader", "MyGUI_OpenGLES_VP.glsl", "Msdf_OpenGLES_FP.glsl");
#endif

So I can put:

        // MsdfFontShader uses custom fragment program and default vertex program
#if defined(MYGUI_OGRE_PLATFORM)
        MyGUI::RenderManager::getInstance().setMaterial("MyGUI/MsdfFontShader")
#elif defined(MYGUI_OPENGL_PLATFORM)

I don't know why you want the materials to be set in code.

I thought it would be best to have material definitions:

This way is much more simple to pass the parameters to the shaders and you can modify the shaders and the material definitions in the files instead of in the code.

I think that you can even use the shaders already defined for the other platforms: DirectX9/11, OpenGL/3+.

Another thing I wanted to ask is what is this variable: YFlipScale?

@paroj removed it from the code and it had no effect.

Altren commented 3 years ago

registerShader is a pure virtual function of the RenderManager, but it's ok to remove it from there and have different naming as you suggested. But keep in mind, that registerShader just adds "shader" to the list of available "shaders", so yours setMaterial would not work in the MsdfFont demo. The MsdfFontShader name is used in the Media/Demos/Demo_MsdfFont/MsdfFont.xml, that's why we register shader instead of explicitly setting it in code. It is set in .xml.

I don't know why you want the materials to be set in code.

I don't want to have materials be set in code, but I want to register new material into MyGUI so that it can be accepted as "Shader" font property. Alternatevly we can skip whole registerShader for Ogre platform and just accept any valid Ogre's material name.

YFlipScale was required for the render to texture. As far as I remember some platforms do flip texture upside-down, while other do not. Without that you'll get Ogre's OpenGL3 and DirectX11 have different orientation (OpenGL3 would be upside down). I'm not sure why that's not an issue in the @paroj code. May be RTSS handles that.

Altren commented 3 years ago

To test YFlipScale you need to run UnitTest_RTTLayer (build can be enabled with -DMYGUI_BUILD_UNITTESTS=True cmake flag). If YFlipScale is ignored you'll get upside-down image for OpenGL3 render system.

Just tested for master and removing YFlipScale makes RTT upside-down (so it is required to be set). Can't test paroj code right now.

paroj commented 3 years ago

you can set YFlipScale to ACT_RENDER_TARGET_FLIPPING if you use the mygui shaders. With my approach Ogre transparently handles this my modifying the projection matrix.

Altren commented 3 years ago

paroj solution is better, so having no YFlipScale is ok, if it works.

sercero commented 3 years ago

@Altren I have a question regarding the shader code in OgreRenderManager.cpp

    void OgreRenderManager::registerShader(
        const std::string& _shaderName,
        const std::string& _vertexProgramFile,
        const std::string& _fragmentProgramFile)
    {
        auto iter = mRegisteredShaders.find(_shaderName);
        if (iter != mRegisteredShaders.end())
        {
            delete iter->second;
        }
        mRegisteredShaders[_shaderName] = createShader(_shaderName, _vertexProgramFile, _fragmentProgramFile);
        if (_shaderName == "Default")
            mDefaultShader = mRegisteredShaders[_shaderName];
    }

To me it seems that mDefaultShader will always be the same, so how do I set the shader that is being used by MyGUI then?

Altren commented 3 years ago
registerShader("Defualt", ..., ...);

this would overwrite default shader.

sercero commented 3 years ago

Yes, but I am thinking of Demo_MsdfFont that uses the function:

renderManager.registerShader("MsdfFontShader", "MyGUI_Ogre_VP." + renderManager.getShaderExtension(), "Msdf_Ogre_FP." + renderManager.getShaderExtension());

How do you switch to using the "MsdfFontShader"?

Because in OgrePlatform the shader being used by the Renderer is the mDefault shader which is always the "Default".

So registerShader() just adds the shader to the list of registered shaders but then that shader is never used.

Perhaps I am missing something here...

Altren commented 3 years ago

There is explicit OgreTexture::setShader call for the texture object. For now only ResourceTrueTypeFont and ResourceManualFont support that (using direct setShader call or with "Shader" xml font property which is used in Demo_MsdfFont. It is possible to use it by explicitly getting MyGUI::ITexture, but for now this is not used anywhere. If this would be needed setShader can be exposed for the Widget or somewhere else.

sercero commented 3 years ago

OK, now I understand better. Thanks.

sercero commented 3 years ago

@Altren, I am doing a variety of fixes on the Demos and UnitTests (not many) in order to better be able to test the changes I am doing in OgrePlatform.

My question is if you want to have several PRs for each fix or a big PR with everything.

Altren commented 3 years ago

Several PR would be better if possible, because some might require additional changes or might not be accepted. From the other hand I understand that it takes extra time and sometimes it is not worth to do separate PR for each change. I'd rather suggest to start with 1-2 smaller PRs and if there are no controversal changes then go with big PR.

sercero commented 3 years ago

@Altren

I am almost finished with this.

I'm still having problems with the following:  - OpenGL: Sampler value sampler has not been set This seems to be an OGRE problem (and perhaps something in my Windows system) because it does not matter how simple the shader is and how you pass the sampler value it always complains.

 - Shaders for the models in the Demos. I have made some very simple shaders (just texturing) for the models Robot.mesh and Mikko.mesh so the Demo_Gui works also in OpenGL3+ and DirectX11. I wonder if you are OK with having these very simple shader and perhaps use them also for the other models in the UnitTests. The things I don't like about this approach is that I think there is no common directory to put the shaders so all the shader files will be repeated in each of the directory of the models. And also that there are some things that are still using the FixedFunction (like the smoke particles) and I don't know how to make a shader for that.

 - setShader() in OgreTexture I don't like this approach. Currently setShader() is a function in OgreTexture/OpenGLTexture/DirectX11Texture/etc... The problem is that the shader is actually being changed in the xxxRenderManager and in order for the xxxRenderManager to know about this change it has to poll xxxTexture to inquire wether the shader has changed or not. Currently this poll is being done in doRender() which as far as I know is a function that is being called every frame so it is not very efficient to do this inquiry there. I think that a better approach would be to call directly xxxRenderManager->setShader() and then the RenderManager starts using the new shader (previously registered). This was an if can be taken out of doRender() which should have the least amount of conditionals possible.

Altren commented 3 years ago

OpenGL: Sampler value sampler has not been set

Did you tried version by Paroj (this one https://github.com/MyGUI/mygui/pull/196)? Does it have such issue?

Shaders for the models in the Demos.

There is Media/Common/Scene for the shared data, so it should be just this directory where you need new shaders. Also there is Robot.mesh there (as well as in Media/UnitTests/UnitTest_GraphView), the one in UnitTest_GraphView should be removed (or placed instead of Media/Common/Scene/Robot.mesh, if it is animated).

And also that there are some things that are still using the FixedFunction (like the smoke particles) and I don't know how to make a shader for that.

Where? I don't remember anything like that.

setShader() in OgreTexture

I don't see any place where you can call directly xxxRenderManager->setShader(). If you can suggest any I'm ok with such approach, but I think it is not possible to do so.

sercero commented 3 years ago

Did you tried version by Paroj (this one #196)? Does it have such issue? Yes, I am getting this message no matter what:

  • Using a simple OGRE program with a ver simple shader
  • The OGRE SampleBrowser.
  • Any version I try of the MyGUI code. The sampler is being set anyways because in my tests the texture is seen. And @paroj says that anyway the texture unit 0 is being set always.

There is Media/Common/Scene for the shared data, so it should be just this directory where you need new shaders. Also there is Robot.mesh there (as well as in Media/UnitTests/UnitTest_GraphView), the one in UnitTest_GraphView should be removed (or placed instead of Media/Common/Scene/Robot.mesh, if it is animated).

So there will have to be some reordering because right now not all UnitTests are using that folder, but I might be wrong. I will check better.

Where? I don't remember anything like that.

D:\MyGUI\Sources\mygui\Media\UnitTests\UnitTest_GraphView\smoke.material I will have to check better all the places where OGRE materials are being used. But the thing is that I can only make very simple shaders (or use someone elses) because my knowledge is extremely limited.

I don't see any place where you can call directly xxxRenderManager->setShader(). If you can suggest any I'm ok with such approach, but I think it is not possible to do so.

OK, I put an if in the middle of doRender(), it is not the ideal case but it is consistent with the other RenderManagers.

Altren commented 3 years ago

So there will have to be some reordering because right now not all UnitTests are using that folder, but I might be wrong. I will check better.

All Demos and UnitTests that do 3d render already include that, as far as I remember. For the plane material.

D:\MyGUI\Sources\mygui\Media\UnitTests\UnitTest_GraphView\smoke.material

I'll check that. I guess it might be ok to just remove non-mesh data such as particles.

sercero commented 3 years ago

I don't see any place where you can call directly xxxRenderManager->setShader(). If you can suggest any I'm ok with such approach, but I think it is not possible to do so.

If it is possible to call registerShader() then it is also possible to add a setShader() function and move the function from xxxTexture to xxxRenderManager.

I know it is painful because it means that it will have to change in all the RenderManagers.

If you are so kind can you explain why the shader is being set for the texture and not in the RenderManager?

I guess it makes sense to have a shader per texture, the problem here again is that I don't know very well the general architecture of MyGUI.

For now I have solved it like this:

    void OgreRenderManager::doRender(IVertexBuffer* _buffer, ITexture* _texture, size_t _count)
    {
        MYGUI_ASSERT(_texture != nullptr, "Rendering without texture is not supported");

        OgreTexture* texture = static_cast<OgreTexture*>(_texture);

        auto pass = mRenderable.getTechnique()->getPass(0);
        pass->getTextureUnitState(0)->setTexture(texture->getOgreTexture());

        OgreShaderInfo* shaderInfo = texture->getShaderInfo();

        if(shaderInfo != nullptr && (pass->getVertexProgram() != shaderInfo->vertexProgram || pass->getFragmentProgram() != shaderInfo->fragmentProgram))
        {
            pass->setVertexProgram( shaderInfo->vertexProgram->getName() );
            pass->setFragmentProgram( shaderInfo->fragmentProgram->getName() );

            Ogre::GpuProgramParametersSharedPtr vertexParams = pass->getVertexProgramParameters();
            vertexParams->setNamedConstant("YFlipScale", 1.0f);
        }

        OgreVertexBuffer* buffer = static_cast<OgreVertexBuffer*>(_buffer);
        mRenderable.mRenderOp = *buffer->getRenderOperation();
        mRenderable.mRenderOp.vertexData->vertexCount = _count;

        mSceneManager->_injectRenderWithPass(pass, &mRenderable);

        ++mCountBatch;
    }
sercero commented 3 years ago

@Altren is it OK to use the same name for all the sampler textures, e.g.: samplerTex or samplerTexture?

Otherwise I have to determine the sampler name from the language used (HLSL vs GLSL)

Of course, I would only change the names from the Ogre_xx shaders.

Altren commented 3 years ago

If it is possible to call registerShader() then it is also possible to add a setShader() function and move the function from xxxTexture to xxxRenderManager.

The issue is that setShader is usually called once when the texture is loaded (f.e. when msdf font is created). And then when we do render any widget IVertexBuffer and ITexture object added into buffers and then RenderManager read all data and renders input. We can call setShader somewhere else, but again we would need to store shader info somewhere (so ITexture should have it) and ask something if it require shader (do if per frame). So as I said I don't think that there is a way to do that.

I know it is painful because it means that it will have to change in all the RenderManagers.

That not a big deal for me, and since that part was never released yet it's ok to do major changes if we'll find some better approach.

Code...

It seems that it never resets back to the Default shader, once pass have new shaders those never changed back to default.

@Altren is it OK to use the same name for all the sampler textures, e.g.: samplerTex or samplerTexture?

I guess. Those names are set from Ogre internally, so I don't know if they can be changed. And I don't understand where you want to set those, because current MyGUI code know nothing about this names.

sercero commented 3 years ago

Now I understand that doRender() is being called in sort of layers so the default shader has to be restored. So actually the current design is good, the thing is that what you are doing here is recreating in a way the OGRE material system. Because a texture is getting associated with a set of shaders.

Again, the problem is that I am very ignorant in regards to shaders and rendering. I am only using OGRE and MyGUI to create a game and most of my time goes towards making the game. Hopefully I could abstract myself from the underlying libraries but it seems that it is not possible in this case. Eventually my plan is to learn about shaders, I have already read a book and that is why I am not totally lost.

I guess. Those names are set from Ogre internally, so I don't know if they can be changed. And I don't understand where you want to set those, because current MyGUI code know nothing about this names.

I was setting the sampler value in createShader(), but it is a mistake. The problem here is that I have been misled by this message that is starting to haunt me.

Can you check in your OGRE logs with OpenGL3+ and OpenGL if you have the message: "Sampler value has not been set"?

Thanks.

Altren commented 3 years ago

Yes, I do

sercero commented 3 years ago

I just found a problem with the Msdf HLSL FP shader.

When registerShader() is being called, it creates the shader (if it doesn't exist) with the following parameters (in the case of HLSL):

shaderInfo->fragmentProgram->setParameter("target", "ps_3_0");
shaderInfo->fragmentProgram->setParameter("entry_point", "main");

With the help of Paroj I was able to do this modification with the default shaders because he managed to convert them into ps_3_0, but the Msdf shaders are using functions that are not available in ps_3_0 so the microcode compilation fails.

Can you convert this shader to use functions from ps_3_0?

Another option is to make a change in the way the registerShader() works: Instead of creating a shader, registerShader() gets a material name, that material is defined in a .material script where all the parameters are being set.

So a call to registerShader() only passes the material name and the rest of the parameters are ignored since the vertex and fragment programs are defined in the material.

Also this change requires a new function in OgreTexture called getShaderName() because getShaderInfo() would not work.

What do you think?

With this change you don't have to provide a Msdf shader for ps_3_0 (leaving DirectX9 without Msdf)

sercero commented 3 years ago

Nevermind, I just tested it in DirectX11 and it works despite setting the target as ps_3_0

So, it would be cool to translate the shader to be compatible with DirectX9.

I was trying to do that but was unable to get a replacement for this: sampleTexture.GetDimensions(w, h);

Altren commented 3 years ago

I remember that it was not trivial to make it work on DirectX9, and actually I think it might be OK to leave it not working on some setups. Making some explicit message about that would be better, but I don't feel that it is important to make it work on every platform, especially when this is some older API.

Altren commented 3 years ago

Hi. Any progress with that? If you can't solve some issues, but already added shaders or some other useful stuff it is still worth pushing it to master. Please tell if any help is needed, or I can try to continue you work if you want.

sercero commented 3 years ago

@Altren I created a pull request with a lengthy explanation, did you not see it?

The pull request is this one: #202

sercero commented 3 years ago

In regards to the shaders, I created some shaders for the demos but I think that the best course of action would be to use the RTSS for the demos and unit tests because some things requiere shader knowledge that I don't have.

If the issues mentioned in the pull request get solved then we can merge paroj's changes to use RTSS for the demos and unit tests, it is easy.

Altren commented 3 years ago

Surprisingly the validation warning issue does not happen on linux neither with OpenGL no with OpenGL3+ Ogre renderers. So it is windows specific problem. cc @paroj

sercero commented 3 years ago

@Altren see if you can check with this program the OpenGL function calls.

I am checking it on my side

GLIntercept

Also, I have a forum question regarding this issue

Validation warning! - Sampler value texMap has not been set

Because I even tried with a very basic shader and I am getting that message, so it is not a MyGUI issue.

See if you can reproduce this issue with the SampleBrowser. In my case I can see it with se sample that shows the Normal Map.

sercero commented 3 years ago

@Altren another thing: we should see how @paroj has currently implemented ImGUI into OGRE.

sercero commented 3 years ago

@Altren here is the code that OGRE uses to render ImGUI: Components/Overlay/src/OgreImGuiOverlay.cpp

Is it possible for MyGUI to use the Overlay?

Altren commented 3 years ago

I remember, that Overlays were much slower than current approach.

Altren commented 3 years ago

Oh, well... Solution suggested by paroj is also slower than older master version. Some demos have 10-20% fps decrease, and Demo_ItemBox have 50% performance decrease on OpenGL and OpenGL3 render systems. Edit: well, in release it is "only" 25% slower.

paroj commented 3 years ago

Oh, well... Solution suggested by paroj is also slower than older master version.

you mean the _injectRenderWithPass compared to the previous implementation?

Of course you pay for going through AutoParamDataSource - my guess would be that re-building the WorldViewProjMatrix matrix per draw-call is the culprit.

However, what does 50% slower mean? If you are rendering 1000fps now, but were rendering at 2000fps before thats 0.5ms which is actually irrelevant.

Especially if you consider Amdahl's law - because the actual scene is rendered with the standard Ogre path anyway..

Altren commented 3 years ago

I agree, that it doesn't make sense to compare 1000fps with 2000fps in real applications. But with a more complex setup this actually matters. I do use Ogre in web (emscripten) and there is gui heavy logic, that actually was my bottleneck. And having 400->300 fps frame drop in a simple application means that in a complex one it would matter.