Fix #83 issues #86

topblast · 2018-07-14T11:17:32Z

The renderer textures are looked up everytime a rect is drawn. This is too inefficient. How about storing a user data in the Texture.

The utf8_to_wchart utility has unreachable code (returns).

...

The software renderer texture colour sampling is broken.

This pull request aims to solve the issues you pointed out in #84.
It also includes the update for the SFML2 renderer.

…utf8_to_wchart; Fix software renderer alpha blending bug

billyquith

These don't seem to be the changes I suggested, which was adding a "user data" pointer in the Font and Texture , just like it was before. I think you have tried to separate the two, but I don't see that this is necessary. It was fine before with a void* handle. Other than a cache for the loaded fonts and textures, for cleanup, and lookup of duplicates on load, I don't see a need to store all these details, it just adds extra complication.

billyquith · 2018-07-14T11:40:27Z

source/platform/include/Gwork/Renderers/Software.h

@@ -121,13 +122,15 @@ namespace Gwk

                Color& At(int x, int y)
                {
-                    return (reinterpret_cast<Color*>(m_ReadData.get()))[y * static_cast<int>(width) + x];
+                    unsigned char* color_ptr = m_ReadData.get() + (y * 4 * static_cast<int>(width) + x * 4);


Magic numbers. Why not do that match with Color pointer?

4 bytes, one for each color rgba

So sizeof(Color)? Or (better), just leave it as it was and let the Color* do the work.

Will do, this was a careless mistake from when I was debugging the software renderer.

billyquith · 2018-07-14T11:43:38Z

source/platform/include/Gwork/Utility.h

@@ -87,7 +87,15 @@ namespace Gwk

            static inline wchar_t utf8_to_wchart(char*& in)// Gwk::Utility::Widen too slow


Where is this code from? Another library? If so the copyright notice and license need adding.

This code is a modified version of the answer provided here.

Could we add a comment with a link to that code then please.

billyquith · 2018-07-14T12:05:32Z

source/platform/include/Gwork/Renderers/Allegro5.h

@@ -111,6 +112,8 @@ namespace Gwk

            std::unordered_map<Font, ALFontData> m_fonts;
            std::unordered_map<Texture, ALTextureData> m_textures;
+            std::pair<const Font, ALFontData>* m_lastFont;


Why store the last font here? Why not store an opaque handle in Font, like it was? If the font keeps changing you have to constantly look it up. The only cache you need is to look up the font by family/size, etc, i.e. style.

The reason for avoiding the user data in Font/Texture primarily is to remove data specific to a renderer having the potential to be shared between renderers. One of my use cases for Gwork is an environment where multiple renderers exist. Some of the Gwork objects are shared between these renderers. The user data in Font/Texture does not have a way to determine the renderer that assigned the data or store multiple without recreating every time the renderers change.

I also question the lifespan of user data in Font/Texture.

Multiple renderers?! I'm curious, how so? Multiple instances with the same backend, or multiple backends?

It has always been assumed that an application would embed one renderer. I have thought about having fallback renderers if some features aren't available on other renderers, e.g. if more primitives were added to add a canvas (e.g. drawing of charts etc) it call fallback to software renderer if a particular renderer did not have that features. It's tempting to include something like NanoVG to do something like this.

One example of multiple renderers is in-game overlays. Without knowing which rendering API the game will you, multiple backends are used. However, some game when switching between lobby & battle(for lack of a better word), different rendering APIs are used. Usually different versions of Direct X. And in rare cases, the HUD is rendered in DX 10 and the 3D environment in DX 11.

Ok, this has never been a requirement before. Interesting. I was wondering why the extensive changes and whether is was overkill! I can see your motivation now!

I added #87 so this feature can be mentioned as it is significant. Will have to mention/update docs.

billyquith · 2018-07-14T12:06:25Z

source/platform/include/Gwork/Renderers/SFML2.h

+            void DrawTexturedRect(const Gwk::Texture& texture, Gwk::Rect targetRect, float u1 = 0.0f,
+                float v1 = 0.0f, float u2 = 1.0f, float v2 = 1.0f) override;
+
+            Gwk::Color PixelColor(const Gwk::Texture& texture,


Please indent params at open bracket level, for consistency.

billyquith · 2018-07-14T12:14:28Z

source/platform/renderers/Allegro5/Allegro5.cpp

@@ -126,7 +129,7 @@ Font::Status Allegro::LoadFont(const Font& font)
    {
        ALFontData fontData;
        fontData.font = deleted_unique_ptr<ALLEGRO_FONT>(afont, [](ALLEGRO_FONT* mem) { if (mem) al_destroy_font(mem); });
-        m_fonts.insert(std::make_pair(font, std::move(fontData)));
+        m_lastFont = &(*m_fonts.insert(std::make_pair(font, std::move(fontData))).first);


std::move has no effect here since you have already constructed fontData?

fontData cannot be copied, the std::move forces the move constructor to be called, swapping the properties between the constructed fontData and the unconstructed one in std::pair.

billyquith · 2018-07-14T12:15:39Z

source/platform/renderers/OpenGL/OpenGL.cpp

@@ -44,7 +44,7 @@ namespace Renderer
 // See "Font Size in Pixels or Points" in "stb_truetype.h"
 static constexpr float c_pointsToPixels = 1.333f;
 // Arbitrary size chosen for texture cache target.
-static constexpr int c_texsz = 512;
+static constexpr int c_texsz = 800; // Texture size too small for wchar_t characters but stbtt_BakeFontBitmap crashes on larger sizes


Does this have to be a power of 2?

Not necessarily, this integer serves as both the width and height of the texture for a font. I increased the number because function stbtt_BakeFontBitmap() returns the highest Y value that was used on success or the negative of the index it failed at. It turns out that 512 is too small for the default font, all characters cannot be placed on the texture.

I was commenting on the crash. I wondered if texture size had to be power of 2 or a multiple of something, causing a platform specific issue.

Does FontStash use multiple cache textures? I.e. when one fills up, create another. I know Allegro does. Perhaps the "font module" needs to chain multiple pages instead of relying on a fixed size.

billyquith · 2018-07-14T12:20:16Z

source/platform/renderers/SFML2/SFML2.cpp

 # include <GL/gl.h>
 #endif

 #include <cmath>
-#include <unistd.h>


Why has this been removed?

This file could not be found, in visual studio. I meant to add ifndef _WIN32 around it as it appears to be for POSIX systems.

Interesting. I didn't know this had broken the Windows build of SFML2. I work on a Mac.

billyquith · 2018-07-14T12:31:50Z

There is a build breakage due to thread_local being used. It seems this is not portable?

topblast added 2 commits July 14, 2018 07:08

Cache Font & Texture data in renderers to reduce lookups; Fix bug in …

714e33d

…utf8_to_wchart; Fix software renderer alpha blending bug

Update SFML2 Renderer

f920ad2

billyquith requested changes Jul 14, 2018

View reviewed changes

topblast added 4 commits July 14, 2018 10:46

Code formatting

ee87bd1

Xcode version specific for thread_local

b833659

Fixed __apple_build_version__ bug

6a017cd

Revert SWTextureData::At(int, int) functions

b225ed4

billyquith approved these changes Jul 14, 2018

View reviewed changes

billyquith merged commit 7816f08 into billyquith:gwork Jul 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #83 issues #86

Fix #83 issues #86

topblast commented Jul 14, 2018

billyquith left a comment

billyquith Jul 14, 2018

topblast Jul 14, 2018

billyquith Jul 14, 2018

topblast Jul 14, 2018

billyquith Jul 14, 2018

topblast Jul 14, 2018

billyquith Jul 14, 2018

billyquith Jul 14, 2018

topblast Jul 14, 2018

billyquith Jul 14, 2018

billyquith Jul 14, 2018

topblast Jul 14, 2018

billyquith Jul 14, 2018

billyquith Jul 14, 2018

billyquith Jul 14, 2018

billyquith Jul 14, 2018

topblast Jul 14, 2018

billyquith Jul 14, 2018

topblast Jul 14, 2018

billyquith Jul 14, 2018

billyquith Jul 14, 2018

topblast Jul 14, 2018

billyquith Jul 14, 2018

billyquith commented Jul 14, 2018

		@@ -87,7 +87,15 @@ namespace Gwk

		static inline wchar_t utf8_to_wchart(char*& in)// Gwk::Utility::Widen too slow

Fix #83 issues #86

Fix #83 issues #86

Conversation

topblast commented Jul 14, 2018

billyquith 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

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

billyquith commented Jul 14, 2018