1

I sent my application for testing to several people. First tester has the same result as mine, but other twos have something strange. For some reason, image, which should be in original size at lower left corner, they see as stretched to fullscreen. Also they don't see GUI elements (however, buttons work if they are found by mouse). I will make reservation, that this isn't stretched image overlaps the buttons, I sent them version with transparent image and the buttons still aren't drawn. For GUI drawning I use Nuklear library. I will give screenshots and code that's responsible for positioning problem image. What can cause that?

[ Good behavior / Bad behavior ]

int width, height;
{
    fs::path const path = fs::current_path() / "gamedata" / "images" / "logo.png";
    unsigned char *const texture = stbi_load(path.u8string().c_str(), &width, &height, nullptr, STBI_rgb_alpha);
    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, width, height, 0, GL_RGBA, GL_UNSIGNED_BYTE, texture);
    stbi_image_free(texture);
}

...

{
    float const x = -1.0f + width * 2.0f / xResolution;
    float const y = -1.0f + height * 2.0f / yResolution;

    float const vertices[30] = {
        /* Position */        /* UV */
        -1.0f, -1.0f, 0.0f,   0.0f, 0.0f,
        -1.0f,  y,    0.0f,   0.0f, 1.0f,
        x,      y,    0.0f,   1.0f, 1.0f,
        -1.0f, -1.0f, 0.0f,   0.0f, 0.0f,
        x,     -1.0f, 0.0f,   1.0f, 0.0f,
        x,      y,    0.0f,   1.0f, 1.0f
    };

    glBufferData(GL_ARRAY_BUFFER, 30 * sizeof(float), vertices, GL_STATIC_DRAW);
}

[ UPDATE ]

By trial and error, I realized that problem is caused by classes that are responsible for rendering the background and logo, and both of them are wrong. Separately they work as it should, but as soon as something else is added to game loop, everything breaks down.

glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
background.render();
glEnable(GL_BLEND);
glDepthMask(GL_FALSE);
logo.render();
nk_glfw3_render();
glDepthMask(GL_TRUE);
glDisable(GL_BLEND);

I wrote these classes myself, so most likely I missed something. Having found mistake in one, there's another, cuz they're almost identical. So far I can't determine, what exactly in these classes are wrong…

[ Background.hpp / Background.cpp ]

  • Please only tag the language you are using – Christian Gibbons Jul 02 '18 at 14:59
  • @ChristianGibbons Indeed, I'm sorry. –  Jul 02 '18 at 15:05
  • The image in Badbehavior1 looks a lot like Badbehavior2. – Olivier Sohn Jul 02 '18 at 15:05
  • Is that some testers use Intel CPUs others AMDs? – Maxim Egorushkin Jul 02 '18 at 15:05
  • Could it be that a glViewport is missing just before rendering the icon and gui elements? – Olivier Sohn Jul 02 '18 at 15:07
  • @MaximEgorushkin I will ask them right now. –  Jul 02 '18 at 15:11
  • @iMajuscule No, glViewport specified. –  Jul 02 '18 at 15:11
  • @Bileslaw I had a similar bug where just changing the windowing system was making the app look different, because the windowing system was doing glViewports on its own, and I was assuming it would not do any.What do you mean by "glViewport specified" exactly? – Olivier Sohn Jul 02 '18 at 15:16
  • @iMajuscule I'm refactored code of windowing system (it's single header) and removed glViewport from there. I mean that glViewport called before the game loop and never changes after. –  Jul 02 '18 at 15:22
  • @iMajuscule After all, even if that were so, wouldn't it work incorrectly on all computers? –  Jul 02 '18 at 15:23
  • @Bileslaw on all computers that have the same (version of) windowing system, yes. But with 2 computers having 2 different windowing systems, you can see a bug in one, and not in the other. – Olivier Sohn Jul 02 '18 at 15:24
  • Have you tried specifying mag and min mipmap filter to image? If not please try and see if it changes anything. – Paritosh Kulkarni Jul 02 '18 at 16:01
  • @ParitoshKulkarni Both are set to GL_LINEAR. –  Jul 02 '18 at 16:06
  • 1
    Some of the advice in these comments is not viable. Mipmaps will not change anything. Bad use of `glViewport` would not explain the behavior. I suggest disabling parts of your rendering pipeline until you can get it to work on all computers, this way you can identify which particular other draw part of your code or the GUI code is leaving your OpenGL context in an unexpected state. Once that is isolated, the answer may be obvious or you can post the exact snipped of code causing the problem. As it is, the problem is simply not narrowed down enough for us to answer. – Dietrich Epp Jul 02 '18 at 16:07
  • @DietrichEpp Reasonable remark. It's pity that I don't have computer on which problem was visible directly at hand. This could simplify task. But, apparently, there's no other way out, except how to send program to testers after each change. As soon as I find a problem, I'll write here. –  Jul 02 '18 at 16:19
  • Run the program in CodeXL, activate break-on-error, and compare all state variables (used). Could be goofy defaults or unimplemented features. – Andreas Jul 02 '18 at 18:06
  • @DietrichEpp I added problem code to question. Please, check it. –  Jul 02 '18 at 21:19

1 Answers1

2

I found some errors in the posted code. In dramatic fashion, I will reveal the culprit last.

NULL used as an integer

For example,

glBindTexture(GL_TEXTURE_2D, NULL); // Incorrect

The glBindTexture function accepts an integer parameter, not a pointer. Here is the correct version:

glBindTexture(GL_TEXTURE_2D, 0); // Correct

This also applies to glBindBuffer and glBindVertexArray.

Nullary constructor is defined explicit

The explicit keyword only affects unary constructors (constructors which take one parameter). It does not affect constructors with any other number of parameters.

explicit Background() noexcept; // "explicit" does not do anything.

Background() noexcept; // Exact same declaration as above.

Constructor is incorrectly defined as noexcept

The noexcept keyword means "this function will never throw an exception". However it contains the following code which can throw:

new Shader("image")

According to the standard, this can throw a std::bad_alloc. So the noexcept annotation is incorrect. See Can the C++ `new` operator ever throw an exception in real life?

On a more practical note, the constructor reads an image from disk. This is likely to fail, and throwing an exception is a reasonable way of handling this.

The noexcept keyword is not particularly useful here. Perhaps the compiler can generate slightly less code at the call site, but this is likely to make at most an infinitesimal difference because the constructor is cold code (cold = not called often). The noexcept qualifier is mostly just useful for choosing between different generic algorithms, see What is noexcept useful for?

No error handling

Remember that stdbi_load will return NULL if an error occurs. This case is not handled.

Rule of three is violated

The Background class does not define a copy constructor or copy assignment operator even though it defines a destructor. While this is not guaranteed to make your program incorrect, it is kind of like leaving a loaded and cocked gun on the kitchen counter and hoping nobody touches it. This is called the Rule of Three and it is simple to fix. Add a deleted copy constructor and copy assignment operator.

// These three go together, either define all of them or none.
// Hence, "rule of three".
Background(const Background &) = delete;
Background &operator=(const Background &) = delete;
~Background();

See What is The Rule of Three?

Buffer is incorrectly deleted

Here is the line:

glDeleteBuffers(1, &VBO);

The short version... you should move this into Background::~Background().

The long version... when you delete the buffer, it is not removed from the VAO but the name can get reused immediately. According to the OpenGL 4.6 spec 5.1.2:

When a buffer, texture, or renderbuffer object is deleted, it is ... detached from any attachments of container objects that are bound to the current context, ...

So, because the VAO is not currently bound, deleting the VBO does not remove the VBO from the VAO (if the VAO were bound, this would be different). But, section 5.1.3:

When a buffer, texture, sampler, renderbuffer, query, or sync object is deleted, its name immediately becomes invalid (e.g. is marked unused), but the underlying object will not be deleted until it is no longer in use.

So the VBO will remain, but the name may be reused. This means that a later call to glGenBuffers might give you the same name. Then, when you call glBufferData, it overwrites the data in both your background and your logo. Or, glGenBuffers might give you a completely different buffer name. This is completely implementation-dependent, and this explains why you see different behavior on different computers.

As a rule, I would avoid calling glDeleteBuffers until I am done using the buffer. You can technically call glDeleteBuffers earlier, but it means you can get the same buffer back from glGenBuffers.

Dietrich Epp
  • 205,541
  • 37
  • 345
  • 415
  • Thank you for such detailed answer, but there one more question. Why we can't use NULL as integer? I thought it's just alias for zero, unlike nullptr. –  Jul 03 '18 at 11:18
  • @Bileslaw: You can’t use `NULL` as an integer because your code will be read by humans, not just by the compiler. It’s such a flagrant style violation that I don’t think there should be any controversy over whether it is appropriate. – Dietrich Epp Jul 03 '18 at 14:14