Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move renderer specific data in Gwk::Font & Gwk::Texture to renderers. #83

Merged
merged 18 commits into from
Jul 12, 2018
Merged

Move renderer specific data in Gwk::Font & Gwk::Texture to renderers. #83

merged 18 commits into from
Jul 12, 2018

Conversation

topblast
Copy link
Contributor

@topblast topblast commented Jul 8, 2018

In this push, the structures Gwk::Font & Gwk::Texture changed to be independent of every renderer. To allow a change, the Gwk::ResourceLoader class was changed to an interface, which will be the base of the class Gwk::Renderer::Base.

This update enables:

  • A Gwk::Font/Gwk::Texture to be shared between renderers
  • Pass ownership of renderer resources to the renderer
  • Use of const references for Gwk::Font & Gwk::Texture.
  • Separate objects of Gwk::Font/Gwk::Texture will share the same 'data' in the renderer because the hash is the same.

topblast and others added 18 commits July 7, 2018 21:26
In this push, the structures Gwk::Font & Gwk::Texture changed to be independent of every renderer. To allow a change, the Gwk::ResourceLoader class was changed to an interface, which will be the base of the class Gwk::Renderer::Base.

This update enables:
 - A Gwk::Font/Gwk::Texture to be shared between renderers
 - Pass ownership of renderer resources to the renderer
 - Use of const references of Gwk::Font & Gwk::Texture.
 - Seperate objects of  Gwk::Font/Gwk::Texture will share the same 'data' in the renderer because the hash is the same.

if (data == nullptr)
// at this point, the font is garented created
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guaranteed 🤙

Copy link
Owner

@billyquith billyquith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite a big change. Thanks for the work.

Copy link
Owner

@billyquith billyquith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite a change. I guess it makes sense if the renderer owns the device though. I think in the example the device is created and passed to the renderer and so the resources were kept separate.

I was thinking by keeping the resource loader separate the system would be more "componentised" and easier to customise if embedded.

@topblast
Copy link
Contributor Author

Oh, that is true. I was thinking the device and the renderer shared the same lifespan because of the *Context functions (InitializeContext, ShutdownContext, etc) being used with the Gwk::WindowProvider.

With the resource loader, which class will own font & texture data, the resource loader or the renderer. In either case, does the resource loader have a means of passing the required data to the renderer. Each renderer & customised resource loader may have different data structures

@billyquith
Copy link
Owner

The idea with the ResourceLoader is that you might pass in a different one depending on your use, so during dev you might load from disk, and then a release build might load from a (compressed, encrypted) package. If the renderer becomes the ResourceLoader this is no longer possible, unless you have multiple renderers, but I don't think that makes sense! - I guess some sort of "storage abstraction" could be added instead.

There are some real hanging design issues from the original GWEN. As you have pointed out: who owns the fonts, etc? I guess once this is resolved it needs documenting. I already added an architectural overview in the docs.

I don't want to discard this work. It just isn't quite what I had in mind.

@billyquith
Copy link
Owner

*Context functions (InitializeContext, ShutdownContext, etc) being used with the Gwk::WindowProvider.

I was also thinking of ditching the WindowProvider stuff to simplify things. This is an embeddable API and I'd expect the Window/context to already exist. I think that was added so that you might create tools, but I think I'd rather add an extension library for all that, perhaps containing more utilities. I'm also trying to avoid this library turning into Qt/a cross platform library.

@topblast
Copy link
Contributor Author

That is true, my previous use cases for GWEN required changes to texture, to support loading from memory.
With that in mind, I will update the approach from this request. Please clarify, if these align with your goals for the project and any additionals may have.

What I currently have in mind is a templated resource loader which either own the font/texture data or eventually pass ownership to a renderer. Like a Gwork, the renderer will accept a ResourceLoader, but it will specify the type of font & texture data it is working it. Desperately avoiding possible functions like RenderText() & SetActiveTexture() being in the resource loader.
TextureData and FontData structures would be updated to be public facing

template <typename FontData, typename TextureData>
class IResourceLoader
{
public:

    //! Types of notification that the loader might want to know about.
    ...
    virtual Gwk::Font::Status LoadFont(const Gwk::Font& font) = 0;
    virtual void FreeFont(const Gwk::Font& font) = 0;
    virtual bool Exist(const Gwk::Font& font) const noexcept = 0;
    // 1. Resource Loader Owns font data
    virtual const FontData& GetFontData(const Gwk::Font& font) const = 0;
    // OR
    // 2. Resource Loader temporary owns font data until renderer claims it
    virtual FontData ReleaseFontData(const Gwk::Font& font) = 0;

    virtual Gwk::Texture::Status LoadTexture(const Gwk::Texture& texture) = 0;
    virtual void FreeTexture(const Gwk::Texture& texture) = 0;
    virtual Gwk::TextureData GetBaseTextureData(const Gwk::Texture& texture) const noexcept = 0;
    virtual bool Exist(const Gwk::Texture& texture) const noexcept = 0;
    // 1. Resource Loader owns texture data
    virtual const TextureData& GetTextureData(const Gwk::Texture& texture) const = 0;
    // OR
    // 2. Resource Loader temporary owns texture data until renderer claims it
    virtual TextureData ReleaseTextureData(const Gwk::Texture& texture) = 0;

    virtual void Notify(NotificationType msg) {}
};

@billyquith
Copy link
Owner

I started an issue to discuss. Before baking in more problems, lets look at this a bit more. What are we trying to solve? #84

@billyquith billyquith changed the base branch from gwork to develop July 12, 2018 19:32
@billyquith billyquith changed the base branch from develop to gwork July 12, 2018 19:47
Copy link
Owner

@billyquith billyquith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets include this. There are plenty of other areas that need work.

@billyquith billyquith merged commit e98075d into billyquith:gwork Jul 12, 2018
billyquith added a commit that referenced this pull request Jul 14, 2018
Fix #83 issues
- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants