Reworked Base texture class #76

Quaker762 · 2017-02-01T13:50:41Z

Textures can now be bound for operations by OpenGL, as well as the
ability to be loaded by the new Virtual File System we have. Textures
are stored as their pixel array in a vector.

Currently, 8-bit textures aren't supported because for reasons that
elude me, they are palletted, and I have no clue as to where the
pallete is (or what data it contains).

The header was also fixed up, as it was missing a 16-bit unused value,
which was distorting the number of bytes passed to the Virtual File
System to load the texture.

z33ky

The texture also needs to be deleted at some point.

Have you considered bindless textures? I don't know if that is something we do want to use, it's just something that I've heard about and lingers in my mind.

z33ky · 2017-02-02T10:22:19Z

source/SH3/graphics/texture.cpp

+        Log(LogLevel::WARN, "sh3_texture::Load( ): Header only read %d bytes of data! It should have been %d", ret, sizeof(sh3_texture_header));
+
+    if(header.bpp == 8)
+        die("8-bit textures are currently unsupported!");


I think die() is a little drastic. We should warn here and get things rendering, even if it's messed up at the moment.
We could also check for other bpps we don't expect.

I think die() is a little drastic

I thought so too, but the screen will end up looking like vomit as you can see here. I'll remove it and log for now though.

z33ky · 2017-02-02T10:22:53Z

source/SH3/graphics/texture.cpp

+    sh3_arc_vfile file(mft, filename);
+    sh3_arc_vfile::read_error e;
+
+    int ret = file.ReadData(&header, sizeof(sh3_texture_header), e); // Read in the header


Should be std::size_t.
I"m also in favor of using sizeof(header) (generally sizeof(variable) where applicable) because the intention here is to read the size of the variable header, which just happens to be a sh3_texture_header here.

I"m also in favor of using sizeof(header) (generally sizeof(variable)

I'll do this from now on then. I think the whole sizeof(typename) has stuck from not only books I've read, but other peoples' code I've looked through.

This is still missing.

z33ky · 2017-02-02T10:28:17Z

source/SH3/graphics/texture.cpp

-    {
-        die("sh3_graphics::texture::Load( ): Unable to find file %s!", filename.c_str());
-    }
+    data.resize(header.texSize);


We could check that texSize == texWidth * texHeight * bpp. That should apply, right?
If it doesn't we should make sure that there's no out-of-bounds read from data.

As in an assertion??

Assertions should only be present in debug mode, so I think this should be a regular check & log.

This is still missing.

z33ky · 2017-02-02T10:29:58Z

source/SH3/graphics/texture.cpp

+    // Now actually create a logical texture with OpenGL on the gpu
+    glGenTextures(1, &tex);
+
+    glBindTexture(GL_TEXTURE_2D, tex);


This could use Bind() and later Unbind(). Though apart from being shorter I don't see much benefit.

This could use Bind() and later Unbind()

I have this weird habit of not using class functions inside other class functions... Strange I know..

Quaker762 · 2017-02-02T12:30:54Z

The texture also needs to be deleted at some point.

Lmao

Have you considered bindless textures? I don't know if that is something we do want to use, it's just something that I've heard about and lingers in my mind.

I had never even heard of these until I googled them. It seems a little bit weird though (and the Khronos site even mentions it's a bit complex here)
I feel like binding makes a little bit more sense. You bind the texture you want to do something on, perform it, and then let it free. They'd be cool to look into and implement though, as long as they're not overly complex haha.

Quaker762 · 2017-02-02T12:35:36Z

Also I just realised I should add texture units into bind, so we can bind to different texture units for different textures as it explained here

z33ky · 2017-02-02T13:15:26Z

include/SH3/graphics/texture.hpp

         */
-        load_error Load(const std::string& filename, sh3_arc& arc);
+         void Bind(GLenum textureUnit);


1 space too many

z33ky · 2017-02-02T13:16:09Z

source/SH3/graphics/texture.cpp

+        Log(LogLevel::WARN, "sh3_texture::Load( ): Header only read %d bytes of data! It should have been %d", ret, sizeof(sh3_texture_header));
+
+    if(header.bpp == 8)
+        die("8-bit textures are currently unsupported!");


die() and Log()?

Crap I forgot to change this...

z33ky · 2017-02-02T13:19:01Z

source/SH3/graphics/texture.cpp

+    int ret = file.ReadData(&header, sizeof(sh3_texture_header), e); // Read in the header
+
+    if(ret != sizeof(sh3_texture_header))
+        Log(LogLevel::WARN, "sh3_texture::Load( ): Header only read %d bytes of data! It should have been %d", ret, sizeof(sh3_texture_header));


We may want to fallback on a dummy texture when this happens, since it would be very cumbersome to ensure we don't read uninitialized struct fields.
For now this would be a good place to die(), though add a //TODO that we want to use a fallback texture in the future.

z33ky · 2017-02-02T13:24:50Z

I feel like binding makes a little bit more sense. You bind the texture you want to do something on, perform it, and then let it free. They'd be cool to look into and implement though, as long as they're not overly complex haha.

I find the stateful implementation weirder than the stateless one. The stateless version is OOP-like do_stuff(thingToDoStuffOn, parameters...); whereas the stateful has this verbose pattern of

make_thing_active(thingToDoStuffOn);
do_stuff(parameters...);
//sometimes: make_thing_inactive(thingToDoStuffOn);

Even worse if you made some function-calls between make_thing_active and do_stuff, because you have to hope that thingToDoStuffOn is still active, or you need to re-activate it just in case.
At least this is my understanding of the concept, without having read anything about the actual API.

Quaker762 · 2017-02-02T13:39:33Z

I find the stateful implementation weirder than the stateless one

Aren't programming paradigms and semantics fun haha.

Even worse if you made some function-calls between make_thing_active and do_stuff, because you have to hope that thingToDoStuffOn is still active, or you need to re-activate it just in case.

Very true, though the chance of that happening are pretty slim with the amount we're reviewing each others code. Though I do see the problem in this. OpenGL wont shit itself if this happens, so your calls just silently fail or even worse corrupt something.

At least this is my understanding of the concept, without having read anything about the actual API.

It seems like it. The only other issue I have is this is an ARB extension, which I assume most modern graphics cards support, but the chances of this being run on someone's grandma's PC is pretty high haha.

z33ky · 2017-02-02T14:24:02Z

The only other issue I have is this is an ARB extension, which I assume most modern graphics cards support, but the chances of this being run on someone's grandma's PC is pretty high haha.

I might have an idea how we can support both in a uniform API. I'll try that when this PR is ~~merged~~ rebased ;).

Quaker762 · 2017-02-03T03:46:01Z

I'll try that when this PR is merged rebased ;)

What do you need me to rebase?? Or is this the new workflow method you were talking about in #72 haha?

z33ky · 2017-02-03T09:30:07Z

Or is this the new workflow method you were talking about in #72 haha?

Yeah. The rebase will happen automatically if there's no conflicts.

Quaker762 · 2017-02-03T12:42:33Z

The rebase will happen automatically if there's no conflicts.

How the hell does that work!?!?!

z33ky · 2017-02-07T12:38:53Z

See #76 (review)

z33ky · 2017-02-08T08:31:26Z

source/SH3/graphics/texture.cpp

+
+    glBindTexture(GL_TEXTURE_2D, tex);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, header.texWidth, header.texHeight, 0, GL_BGRA, GL_UNSIGNED_BYTE, &data[0]);
+    glBindTexture(GL_TEXTURE_2D, 0); // Unbind this texture


Should this restore the previously bound texture (can be obtained via glGetIntegerv(GL_TEXTURE_BINDING_2D, &prevTex); I think)?

I don't quite understand why we'd want to bind the previous texture? This only changes a few parameters for this texture during a load. When the actual game is running, textures are bound and unbound when they're drawn (or rather, bound to different texture units and drawn) .

You mean they'll be (re-)bound per frame? That won't be an issue then.

Yeah they have to be to be drawn. Basically (how I'm thinking it'll work), is each mesh has a texture (*.tex). Everything is loaded before the scene is rendered. During the render loop, we transverse a list containing each mesh. Its VBO and texture is bound and drawn etc.

When you generate a texture I'm pretty sure it's recommended you unbind it. Otherwise you could add a bunch of stuff to another texture without realising it.

z33ky · 2017-02-08T08:32:00Z

source/SH3/graphics/texture.cpp

-    {
-        die("sh3_graphics::texture::Load( ): Unable to find file %s!", filename.c_str());
-    }
+    data.resize(header.texSize);


This is still missing.

z33ky · 2017-02-08T08:32:41Z

source/SH3/graphics/texture.cpp

+    sh3_arc_vfile file(mft, filename);
+    sh3_arc_vfile::read_error e;
+
+    int ret = file.ReadData(&header, sizeof(sh3_texture_header), e); // Read in the header


This is still missing.

Textures can now be bound for operations by OpenGL, as well as the ability to be loaded by the new Virtual File System we have. Textures are stored as their pixel array in a vector. Currently, 8-bit textures aren't supported because for reasons that elude me, they are palletted, and I have no clue as to where the pallete is (or what data it contains). The header was also fixed up, as it was missing a 16-bit unused value, which was distorting the number of bytes passed to the Virtual File System to load the texture.

z33ky · 2017-02-08T13:15:21Z

source/SH3/graphics/texture.cpp

-    if(arc.LoadFile(filename, pixbuff) == ARC_FILE_NOT_FOUND)
+    data.resize(header.texSize);
+
+    if(header.texSize == header.texWidth * header.texHeight * header.bpp)


Do an early return, i.e. instead of

if(x) { A; } else { B; return; }

do

if(!x) { B; return; } A;

Once we upload the texture data to the Graphics Card, it is a waste of conventional memory to keep this around...

Quaker762 requested a review from z33ky February 1, 2017 13:50

z33ky requested changes Feb 2, 2017

View reviewed changes

Quaker762 force-pushed the textures branch from ee8f472 to 45c3cfa Compare February 2, 2017 12:49

z33ky requested changes Feb 2, 2017

View reviewed changes

Quaker762 force-pushed the textures branch from 994aa1a to d6cf505 Compare February 2, 2017 13:36

Quaker762 added Engine enhancement OpenGL labels Feb 6, 2017

Quaker762 self-assigned this Feb 6, 2017

Quaker762 requested a review from z33ky February 6, 2017 13:05

Quaker762 force-pushed the textures branch from 3a67753 to 1d5bd1e Compare February 8, 2017 03:34

z33ky requested changes Feb 8, 2017

View reviewed changes

z33ky mentioned this pull request Feb 8, 2017

Initial bmp loader #81

Merged

Quaker762 force-pushed the textures branch from 0bafb5c to 6a68050 Compare February 8, 2017 13:10

z33ky reviewed Feb 8, 2017

View reviewed changes

Removed data vector from texture class

56eb0dc

Once we upload the texture data to the Graphics Card, it is a waste of conventional memory to keep this around...

Quaker762 force-pushed the textures branch from 316f8b6 to 56eb0dc Compare February 9, 2017 03:29

z33ky approved these changes Feb 9, 2017

View reviewed changes

z33ky merged commit f38ab55 into Palm-Studios:master Feb 9, 2017

Quaker762 deleted the textures branch February 10, 2017 01:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reworked Base texture class #76

Reworked Base texture class #76

Quaker762 commented Feb 1, 2017

z33ky left a comment

z33ky Feb 2, 2017

Quaker762 Feb 2, 2017

z33ky Feb 2, 2017

Quaker762 Feb 2, 2017

z33ky Feb 8, 2017

Quaker762 Feb 8, 2017

z33ky Feb 2, 2017

Quaker762 Feb 2, 2017

z33ky Feb 2, 2017

z33ky Feb 8, 2017

z33ky Feb 2, 2017

Quaker762 Feb 2, 2017

Quaker762 commented Feb 2, 2017

Quaker762 commented Feb 2, 2017

z33ky Feb 2, 2017

z33ky Feb 2, 2017

Quaker762 Feb 2, 2017

z33ky Feb 2, 2017

z33ky commented Feb 2, 2017 •

edited

Loading

Quaker762 commented Feb 2, 2017

z33ky commented Feb 2, 2017

Quaker762 commented Feb 3, 2017

z33ky commented Feb 3, 2017

Quaker762 commented Feb 3, 2017

z33ky commented Feb 7, 2017

z33ky Feb 8, 2017 •

edited

Loading

Quaker762 Feb 8, 2017

z33ky Feb 8, 2017

Quaker762 Feb 8, 2017

z33ky Feb 8, 2017

z33ky Feb 8, 2017

z33ky Feb 8, 2017

Quaker762 Feb 9, 2017

Reworked Base texture class #76

Reworked Base texture class #76

Conversation

Quaker762 commented Feb 1, 2017

z33ky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quaker762 commented Feb 2, 2017

Quaker762 commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

z33ky commented Feb 2, 2017 • edited Loading

Quaker762 commented Feb 2, 2017

z33ky commented Feb 2, 2017

Quaker762 commented Feb 3, 2017

z33ky commented Feb 3, 2017

Quaker762 commented Feb 3, 2017

z33ky commented Feb 7, 2017

z33ky Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

z33ky commented Feb 2, 2017 •

edited

Loading

z33ky Feb 8, 2017 •

edited

Loading