#2455 closed enhancement (fixed)
Texture System Update
| Reported by: | IronNerd | Owned by: | IronNerd |
|---|---|---|---|
| Priority: | Nice to Have | Milestone: | Alpha 16 |
| Component: | Core engine | Keywords: | |
| Cc: | Patch: |
Description
This is a precursor to changes in OglTex that will be required for the OpenGL 2 update.
The change is mostly refactoring to make the interface more consistent with the rest of the project, but it has some incidental improvements. Free is automatically called in the destructor, making it harder to accidentally leak memory and requiring a mild update to the get_average_colour function. Some sections of code have been made shorter or more readable by the update.
This will probably be due for another (smaller) revisit mid-way through the renderer update, sometime after cubemaps have native support and external references to member variables have been mostly reduced to the codecs.
Attachments (2)
Change History (12)
by , 11 years ago
comment:1 by , 11 years ago
by , 11 years ago
| Attachment: | Tex_v2.patch added |
|---|
comment:2 by , 11 years ago
I booted linux in a VM and managed to reproduce the problem. I've uploaded Tex_v2.patch, which should fix both the warning and the crash. Thanks for the detailed outputs!
comment:3 by , 11 years ago
The second patch seems to compile and run fine (except there still are a few possibly related memory warnings from valgrind). This should be committable after someone more familiar with graphics programming approves the patch logic.
Thanks for the effort you're putting into this!
comment:4 by , 11 years ago
Compiled nicely on my system (using OSX 10.8 sdk). Works in-game. This fits our convention and seems a good change anyway...
Why not go al the way and make "Tex" a class now that it behaves really quite like one?
comment:5 by , 11 years ago
This is just the first stage of the change. I intend to officially change it to a class when its metamorphosis is complete and it's actually used like one. By used as a class, I mean that basically everything is done through meaningful functions and not just get/set the member variables to get the job done.
Step 1 (this): Move all of the major functions inside the struct and make it RAII by default
Step 2: Change the Tex_Codec code to actually use this like a class (coded, but not uploaded for review, as it would make the review too big... about this size of a change again)
Step 3: Overhaul the OglTex implementation, which also currently doesn't really use this like a class (This is actually about 2-3 steps if we want to keep review sizes reasonable)
Step 4: Implement native OpenGL cubemap support and change the rendering code to use that instead of its current hack (which uses tex quite like a struct still)
Step 5: Translate this into a real full-fledged class with private members and a meaningful interface.
As you can see, there is still quite a bit of work to do before we can really elevate Tex to its desired status, and the whole work-chain required is many times larger than this small patch. As soon as this one is in, we can move to reviewing step 2 as I implement step 3.
comment:6 by , 11 years ago
Hmm, so some bad news. I noticed that the minimap colors are distorted when your patch is applied. I'll edit in some examples in a few minutes.
comment:7 by , 11 years ago
Cannot reproduce, I get this blackness only until the textures are loaded. Please flush your cache and try again.
comment:9 by , 11 years ago
| Milestone: | Backlog → Alpha 16 |
|---|
comment:10 by , 10 years ago
IronNerd: Do you think you'll have time to complete any more work on 0AD? Your contributions were very appreciated and we'd love to see you back, even if it's just small patches.
The actual patch style looks good, but I had some issues running/compiling with your patch on linux.
First, a build warning:
==== Building lowlevel (debug) ==== cursor.cpp ogl_tex.cpp tex.cpp tex_dds.cpp tex_tga.cpp tex_png.cpp tex_codec.cpp tex_bmp.cpp tex_jpg.cpp ../../../source/lib/tex/tex_jpg.cpp: In function ‘Status jpg_decode_impl(rpU8, size_t, jpeg_decompress_struct*, Tex*)’: ../../../source/lib/tex/tex_jpg.cpp:502:9: warning: variable ‘ret’ set but not used [-Wunused-but-set-variable] Status ret = INFO::OK; ^ Linking lowlevelThe second issue was a segfault on start. Here is the backtrace from gdb (Gnu DeBugger):
#0 malloc_consolidate (av=av@entry=0x7ffff4408740 <main_arena>) at malloc.c:4088 #1 0x00007ffff40c80e1 in _int_malloc (av=0x7ffff4408740 <main_arena>, bytes=1048576) at malloc.c:3379 #2 0x00007ffff40ca4d0 in __GI___libc_malloc (bytes=1048576) at malloc.c:2859 #3 0x00000000006aac7f in malloc (sz=1048576) at ../../../source/ps/Profile.cpp:534 #4 0x00007fffb5a4bdf3 in ?? () from /usr/lib/x86_64-linux-gnu/dri/fglrx_dri.so #5 0x00007fffb5a4c184 in ?? () from /usr/lib/x86_64-linux-gnu/dri/fglrx_dri.so #6 0x00007fffb5a2fc7a in ?? () from /usr/lib/x86_64-linux-gnu/dri/fglrx_dri.so #7 0x00007fffb5824d6c in ?? () from /usr/lib/x86_64-linux-gnu/dri/fglrx_dri.so #8 0x00007fffb4f837d0 in ?? () from /usr/lib/x86_64-linux-gnu/dri/fglrx_dri.so #9 0x00007fffb4f84a6c in ?? () from /usr/lib/x86_64-linux-gnu/dri/fglrx_dri.so #10 0x00007fffb53b7af5 in ?? () from /usr/lib/x86_64-linux-gnu/dri/fglrx_dri.so #11 0x0000000000a22243 in upload_compressed_level (level=0, level_w=2048, level_h=1024, level_data=0x7fffcb58d080 "\233\255\233\255", level_data_size=1048576, cbData=0x7fffffffd3f0) at ../../../source/lib/res/graphics/ogl_tex.cpp:886 #12 0x0000000000a553b6 in tex_util_foreach_mipmap (w=2048, h=1024, bpp=4, pixels=0x7fffcb58d080 "\233\255\233\255", levels_to_skip=0, data_padding=4, cb=0xa221cc <upload_compressed_level(size_t, size_t, size_t, uint8_t const*, size_t, void*)>, cbData=0x7fffffffd3f0) at ../../../source/lib/tex/tex.cpp:151 #13 0x0000000000a22327 in upload_impl (t=0x7fffea6341a0, fmt=33776, int_fmt=33776, levels_to_skip=0, uploaded_size=0x7fffea634200) at ../../../source/lib/res/graphics/ogl_tex.cpp:903 #14 0x0000000000a22629 in ogl_tex_upload (ht=196611, fmt_ovr=0, q_flags_ovr=0, int_fmt_ovr=0) at ../../../source/lib/res/graphics/ogl_tex.cpp:960 #15 0x000000000076cebd in CTextureManagerImpl::LoadTexture (this=0x1608480, texture=..., path=...) at ../../../source/graphics/TextureManager.cpp:219 #16 0x000000000076d0fe in CTextureManagerImpl::TryLoadingCached (this=0x1608480, texture=...) at ../../../source/graphics/TextureManager.cpp:266 #17 0x000000000076af12 in CTexture::TryLoad (this=0x1856940) at ../../../source/graphics/TextureManager.cpp:557 #18 0x000000000076ae5f in CTexture::GetHandle (this=0x1856940) at ../../../source/graphics/TextureManager.cpp:544 #19 0x00000000007afd8c in CShaderProgram::BindTexture (this=0x241fa20, id=..., tex=...) at ../../../source/graphics/ShaderProgram.cpp:700 ---Type <return> to continue, or q <return> to quit--- #20 0x0000000000990e32 in GUIRenderer::Draw (Calls=..., Z=20) at ../../../source/gui/GUIRenderer.cpp:358 #21 0x00000000009e23d8 in CGUISpriteInstance::Draw (this=0x17e0d80, Size=..., CellID=0, Sprites=..., Z=20) at ../../../source/gui/CGUISprite.cpp:42 #22 0x000000000094cb42 in CGUI::DrawSprite (this=0x16fcb60, Sprite=..., CellID=0, Z=@0x7fffffffd868: 20, Rect=...) at ../../../source/gui/CGUI.cpp:393 #23 0x000000000097a139 in CImage::Draw (this=0x10478b0) at ../../../source/gui/CImage.cpp:55 #24 0x00000000009562ea in GUI<int>::RecurseObject (RR=1, pObject=0x10478b0, pFunc=&virtual table offset 56) at ../../../source/gui/GUIutil.h:320 #25 0x000000000095632d in GUI<int>::RecurseObject (RR=1, pObject=0x17cf620, pFunc=&virtual table offset 56) at ../../../source/gui/GUIutil.h:326 #26 0x000000000095632d in GUI<int>::RecurseObject (RR=1, pObject=0x1792a90, pFunc=&virtual table offset 56) at ../../../source/gui/GUIutil.h:326 #27 0x000000000094ca32 in CGUI::Draw (this=0x16fcb60) at ../../../source/gui/CGUI.cpp:373 #28 0x000000000097edef in CGUIManager::Draw (this=0x15e6c90) at ../../../source/gui/GUIManager.cpp:391 #29 0x00000000006f20df in Render () at ../../../source/ps/GameSetup/GameSetup.cpp:224 #30 0x00000000004a4f0e in Frame () at ../../../source/main.cpp:357 #31 0x00000000004a56d7 in RunGameOrAtlas (argc=1, argv=0x7fffffffdf18) at ../../../source/main.cpp:472 #32 0x00000000004a5a08 in main (argc=1, argv=0x7fffffffdf18) at ../../../source/main.cpp:514Here is the output of valgrind(linux memory debugger) from start to exit:
Not all those warnings/errors are related to your patch.