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

[gui] added colordiffuse attribute on texture definition #1772

Merged
merged 1 commit into from Jun 2, 2013

Conversation

xhaggi
Copy link
Member

@xhaggi xhaggi commented Nov 12, 2012

This PR adds color diffuse attribute for texture definitions.

It's based on the PR #268 and gets rid of the borderdiffuse attribute. also it don't override m_diffuseColor as @jmarshallnz has suggested.

Example:

<texture colordiffuse="FFFFAAFF">texture.png</texture>
<texture colordiffuse="$VAR[colordiffuse]">texture.png</texture>
<texture colordiffuse="$INFO[colordiffuse]">texture.png</texture>

Works for all texture controls, for example textureradioon, textureradiooff, texturefocus, texturenofocus etc.

@xhaggi
Copy link
Member Author

xhaggi commented Nov 14, 2012

it would be nice if this gets into frodo, because without this you need two button controls to use a colordiffuse while button gets focused.

@ronie
Copy link
Member

ronie commented Nov 14, 2012

there are 100's of things that would be nice to have in frodo :)

but reality is, we're way past feature freeze and have beta1 knocking around the corner.

@xhaggi
Copy link
Member Author

xhaggi commented Nov 14, 2012

@ronie i only want a statement if this could get in or not .. sorry if I seem a little impatient ;)

@jmarshallnz
Copy link
Contributor

No, this won't be going in for Frodo.

@xhaggi
Copy link
Member Author

xhaggi commented Nov 14, 2012

@jmarshallnz is there a reason why this is rejected?

@MartijnKaijser
Copy link
Member

@xhaggi
Because we are in feature freeze for a month so nothing will be added until after Frodo release

@xhaggi
Copy link
Member Author

xhaggi commented Nov 14, 2012

thanks

@lemonboy999
Copy link

Now you have some time, you should look at a couple of extra things. Since I put the original PR in, the code base has moved on. The change as submitted does not allow for skin variables to be used in the texture colordiffuse attribute, whereas the control diffusecolor allows this. Also I don't think the original point with one diffuse overriding the other was the real issue, I thought it was about merging the two. The reason the borderdiffuse was rejected was because you could use a bordertexture, however this cannot be applied as an attribute - maybe you could look at this as well.

@xhaggi
Copy link
Member Author

xhaggi commented Nov 15, 2012

@lemonboy999 as you can see in the main description of this PR, it is possible to use $VAR and $INFO .. so i don't know what you mean with skin variables?

@lemonboy999
Copy link

Sorry, I meant that although you can put a $VAR as the colordiffuse attribute, it will only use the value when the control is created and wont update as the $VAR changes. You need to put an extra check in the TextureBase::SetDiffuseColor routine.

@ghost
Copy link

ghost commented Apr 6, 2013

@jmarshallnz post-frodo ping.

@@ -23,6 +23,7 @@
#include "TextureManager.h"
#include "GUILargeTextureManager.h"
#include "utils/MathUtils.h"
#include "GUIInfoTypes.h"

This comment was marked as spam.

@jmarshallnz
Copy link
Contributor

You currently don't call Update() on the CGUIInfoColor, so it won't work with $INFO or $VAR as things stand. I suggest calling it in SetDiffusedColor() would be the best place. Obviously you need to check the Update() routine's return value to return the appropriate changed status.

Exactly how the control's diffuse color and the textures diffuse color interact will need defining. ATM you have the texture's diffuse color overriding the controls. This may not be what is wanted. For example, it might make sense for them to be diffused (multiplied).

@xhaggi
Copy link
Member Author

xhaggi commented Apr 9, 2013

@jmarshallnz thank you for the hints .. i will change it in a few days.

The main reason for this PR is to use it on button controls with <texturefocus colordiffuse="">, so you don't need a second texture or control for highlighting. Well, i don't know if multiply the diffuse color is the best way.

@xhaggi
Copy link
Member Author

xhaggi commented Apr 11, 2013

@jmarshallnz changed up on your hints

@ghost ghost assigned jmarshallnz Apr 11, 2013
@jmarshallnz
Copy link
Contributor

Looks good. Queued for next merge window (May 1->May 10). Before then, it'll need squashing down.

@phil65
Copy link
Contributor

phil65 commented Apr 11, 2013

would it be lot of work to also allow this for radiobutton textures? that would be great, too.

@xhaggi
Copy link
Member Author

xhaggi commented Apr 12, 2013

@phil65 it work's for every texture control .. the comment was an unfortunate choise , I will update it

@ghost
Copy link

ghost commented Apr 15, 2013

Will this also work for the label / altlabel of the togglebutton?

Edit:
Ok, thnx. Either way, awesome work.

@xhaggi
Copy link
Member Author

xhaggi commented Apr 15, 2013

@MassIV only for texture controls .. no labels.

@xhaggi
Copy link
Member Author

xhaggi commented May 16, 2013

@MartijnKaijser something wrong with this, because it was not included in the last merge window

@jmarshallnz
Copy link
Contributor

Nope - my fault for not hitting the button. Apologies.

jmarshallnz added a commit that referenced this pull request Jun 2, 2013
[gui] added colordiffuse attribute on texture definition
@jmarshallnz jmarshallnz merged commit 20b850e into xbmc:master Jun 2, 2013
if (m_alpha != 0xFF) color = MIX_ALPHA(m_alpha, m_diffuseColor);

// diffuse color
color_t color = (m_info.diffuseColor) ? m_info.diffuseColor : m_diffuseColor;

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@xhaggi xhaggi deleted the texture-colordiffuse branch June 2, 2013 08:45
@xhaggi
Copy link
Member Author

xhaggi commented Jun 2, 2013

@jmarshallnz
Copy link
Contributor

Thanks - we'll keep you on :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants