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

TextureGL and GUIFontTTFGL split and cleanup #15549

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

lrusak
Copy link
Contributor

@lrusak lrusak commented Feb 18, 2019

This splits the common GL and GLES classes into two separate classes. The thinking here is to have a common base and have GL/GLES specific stuff in their own class so we can avoid the #ifdef confusion. There is also some cleanup in regards to c-style casts and range based loops.

This will also make it easier in the future when GLES3 get's it's own classes.

There is still some other classes that can be split but that will be in a future PR.

I also seem to have a header loop which causes compiling issues and that's why some of the includes are laid out the way they are. If someone has ideas about how to improve that please let me know.

@lrusak lrusak added Type: Cleanup non-breaking change which removes non-working or unmaintained functionality Type: Improvement non-breaking change which improves existing functionality v19 Matrix labels Feb 18, 2019
@yol
Copy link
Member

yol commented Feb 18, 2019

Looks like a improvement to the status quo on first glance, but I'm not too deep into the GUI stuff - so let me ask a maybe stupid question: Why do we need the type aliases anyway? Can't this be solved with "regular" OO (inheritance/interfaces etc.)? I'm not saying that it's necessarily bad to do it like this, I'd just like to know why it is required.

@lrusak
Copy link
Contributor Author

lrusak commented Feb 18, 2019

Looks like a improvement to the status quo on first glance, but I'm not too deep into the GUI stuff - so let me ask a maybe stupid question: Why do we need the type aliases anyway? Can't this be solved with "regular" OO (inheritance/interfaces etc.)? I'm not saying that it's necessarily bad to do it like this, I'd just like to know why it is required.

I agree, although I'm not sure exactly how to tackle this. Have a look at the questions I posted in slack. Then we can see how to proceed.

@FernetMenta
Copy link
Contributor

It looks like you created a hierarchy with two abstract classes just for the single purpose of sharing code. In addition I see tricks like moving the original abstract methods of the base class (FirstBegin , LastEnd) from private to public. In order to understand the code one have to jump back and forth between three header and three body files.
The shared code between GL and GLES is just a snapshot of the current state and not an optimization of the actual behaviour. A change on the one or other end will most likely result in changes of those three classes.

@lrusak
Copy link
Contributor Author

lrusak commented Feb 19, 2019

It looks like you created a hierarchy with two abstract classes just for the single purpose of sharing code. In addition I see tricks like moving the original abstract methods of the base class (FirstBegin , LastEnd) from private to public. In order to understand the code one have to jump back and forth between three header and three body files.

I don't really understand. Part of inheritance is sharing code is it not? With the purpose of abstracting different codes paths into a common interface. Also, FirstBegin and LastEnd were always public, I did not change that.

The shared code between GL and GLES is just a snapshot of the current state and not an optimization of the actual behaviour. A change on the one or other end will most likely result in changes of those three classes.

So it comes down to it again that you would rather have GL and GLES be completely separate? I mean how is that different than it is currently? Even if you would have to make a change that effects three classes it will still be more organized than it is now and you could even think of the current code being three classes all under the same class.

Regardless, something needs to be done here and if this isn't the correct way forward I'd like to discuss what is the ideal way forward. Using the static defines isn't nice but I'm not sure how to fix it. If you could explain in your mind how you think the best way to improve this code is please share.

We have three render systems which are all mutually exclusive so is there any benefit to having each GUITexture class have it's own name rather than just be called CGUITexture?

For the actual textures there is the possibility that more than one texture type can be used for a given rendering system. For now we have CTexturePi that is used in place of CTextureGL when they could be registered along side each other. I have also created a CTextureGBM class that could do the same thing. So I need some help with the architecture for how this should all fit together. 💭

@lrusak lrusak added the WIP PR that is still being worked on label Feb 19, 2019
@FernetMenta
Copy link
Contributor

Also, FirstBegin and LastEnd were always public, I did not change that.

Take a closer look. You may not have changed this but this does not change the fact that it is a very bad design.

´´´
private:
virtual bool FirstBegin() = 0;
virtual void LastEnd() = 0;
´´´

So it comes down to it again that you would rather have GL and GLES be completely separate?

This is not what I said. Code sharing between architectures can be useful. The way it is done here can be improved.

We have three render systems which are all mutually exclusive so is there any benefit to having each GUITexture class have it's own name rather than just be called CGUITexture?

I don't know if you still have real headless on the roadmap. If this is done right the app would be able to switch from X11/OpenGL to Wayland/GLES for example. The current hacks won't allow this.

private:
void *m_egl_image;
};

using CTexture = CPiTexture;
Copy link
Member

Choose a reason for hiding this comment

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

This conflicts with using CTexture = CTextureGLES; in xbmc/guilib/TextureGLES.h

@@ -248,22 +174,23 @@ void CGUIFontTTFGL::LastEnd()

// Do the actual drawing operation, split into groups of characters no
// larger than the pre-determined size of the element array
for (size_t character = 0; m_vertexTrans[i].vertexBuffer->size > character; character += ELEMENT_ARRAY_MAX_CHAR_INDEX)
for (size_t character = 0; m_vertexTrans[i].vertexBuffer->size > character; character += 1000)
Copy link
Member

Choose a reason for hiding this comment

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

magic number?

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label May 25, 2019
@phunkyfish
Copy link
Contributor

@lrusak do you want to progress this, close it or put it on the backburner?

@phunkyfish phunkyfish added the PR Cleanup: Ask Author PR appears to still be relevant and mergeable. Comment to author to check on status. label Mar 8, 2020
@lrusak
Copy link
Contributor Author

lrusak commented Mar 21, 2020

I would still like to implement this. Something like this will need to be done if we ever want to implement the vulkan renderer.

@phunkyfish phunkyfish added PR Cleanup: Recent Checked as part of PR cleanup. PR has been followed up on recently. and removed PR Cleanup: Ask Author PR appears to still be relevant and mergeable. Comment to author to check on status. labels Mar 23, 2020
@lrusak lrusak requested a review from ksooo as a code owner October 8, 2020 04:04
@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 8, 2020
@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 22, 2020
@jenkins4kodi
Copy link
Contributor

@lrusak this needs a rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR Cleanup: Recent Checked as part of PR cleanup. PR has been followed up on recently. Rebase needed PR that does not apply/merge cleanly to current base branch Type: Cleanup non-breaking change which removes non-working or unmaintained functionality Type: Improvement non-breaking change which improves existing functionality WIP PR that is still being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants