-
Notifications
You must be signed in to change notification settings - Fork 74
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
Nexus - Changes & Fixes #194
Conversation
…replace with recommended Player.HasPerformedSeek(3)
…commended 40px whitespace and add new icon for reset (borrowed from Estuary)
…ces DialogFavourites.xml)
… associated textures (plus a few extras potentially useful for future work).
… lot simpler than removing it as there's various references to it in conditionals etc.
Thanks for this. Dont have k20 yet, but I cherry picked this for future, as I dont know when (if) this will be merged (considering #160) |
@bossanova808 Sorry for the late response. I'll have a look at your PR. Just give me some time please. |
@DaVukovic For sure, no rush on my account. And I'm not claiming it's a perfect PR or anything, more just a collection of my hacks to get things to a reasonable working point in key areas. |
Was wondering why my subtitles looked horrific on 20... 😉 thanks. Hopefully this is reviewed at some point. I've been using Confluence for nearly 20 years now... |
I've started looking at this so it's not all on @DaVukovic First is issue I've spotted is in the Video Calbration window the textures for the selected adjustment are blue, however the new reset texture is white when focused and grey when not focused. The reset button should match this for consistancy. Using a colour picked the blue in the png textures is FF04a7d2 so adjusting the mover id="12" maintains a more consistent look.
Giving |
Jepp, we should either change that or use a completely different Icon. For what I can tell, the Estuary one looks fine so far. I've created another icon, but I would for sure agree on this one as well. @bossanova808 Will you change that in regard of the focused color? It's not a problem if not. I, personally would merge this PR as it is and if you don't want to change it, we can do this ourselves afterwards. In the end it's somewhat important that the color matches the same "blue" as the other sections when submitted to the repo. I've tested the rest (beside the "Hide Legacy CULyrics control"-thingy) and everything looks fine to me. |
@DaVukovic if you're happy with the rest then hit merge. I can do the colour adjustment separately. The colour button and colour picker could probably be improved, but it's more than good enough for now. |
I'm good with that, if you don't mind carrying on from here. Gets things going at least! |
The textures "media/CalibrateSubtitles.png" and "media/icon_reset.png" do have different coloring in the transparent regions. This leads to scaling artifacts. "media/color-back.png" is way to large for a simple unicolor texture. Is there a reason not to use a white low res texture plus diffuse coloring? There seem to be lots of unused textures (media/colors/red.png and similar). |
Also for textures like CalibrateSubtitles.png, I'm always ask myself why there is the need for all this transparent space around it. Is there any particular reason to ship it like this? You could save 80% of resources by cropping. It's cool and all that you want to bring Confluence up to speed, and I won't block this PR. But it would be nice if you could look out for those things. |
To keep things simple, I used the textures from Estuary where possible, making no changes to them. The only one I did anything to was "media/CalibrateSubtitles.png" - and there I simply moved the non-transparent region as per the skinning update guidelines (which I linked to in the PR, and explicitly say to add this whitespace) - so this is as it was, apart from the requested extra 'whitespace' i.e. transparent space. I am not following what you mean about 'different colouring in the transparent regions' - they're transparent, so no colouring? Or you mean the sort of fade off of the textures? That goes back like 15 years, so.... Re: the not needed textures - I had to copy this folder from Estuary to get the new colour picker working. But you're right, I've just tested it, and it indeed only needs the white texture, so it must indeed use that approach. Since the whole folder appeared with that change for Estuary, and I couldn't get the picker to work without the folder, I did assume they were all needed. Sorry, will fix this - (saves a whopping 1.5kb). More generally, if the textures are needlessly large, I'd say take that up with the Estuary people. But we're talking in total a few kb which is not going to have a substantial practical effect on anything here. Have just pushed another commit that removes the unneeded textures and updates the colouring of the reset control as per the above. |
I'm sorry, but Estuary is not really a skin I would advice to get inspiration from in terms of resource usage. A lot of textures are not optimal.
Ok then, just asking. It's just that technically, the whitespace is unnecessary.
The GPU doesn't see transparency, just 4 channels of data. When sampling the texture, it interpolates between the nearest texels without any notion of alpha. In case of the arrow texture, it will sample from the black portion of the texture. You will end up with grey areas where it should be white.
1.5kB is pretty large if you consider that a GPU on an ARM SOC might only have 32kB of L2 cache. If you use such textures here, it will evict other texture data which might still be used elsewhere, resulting in redundant RAM fetches and possibly stalling the GPU. Your "color-back.png" is a big offender. It is around 40kb, which is highly likely to evict other texture data on low-end systems. Even worse, the sampling points are not continuous in their access, skipping texels (the texture is 138x74, while the element is 56x46). This might lead to GPU stalls, as the texture fetching units are usually pretty bad in such situations. In case of the panel where the black texture is used, you can potentially save half the texture bandwidth needed if you use a properly sized texture. And we are talking about up to hundreds of MB/s. A 4 BYTE (1x1) texture might stay in cache indefinitely. In the end, this might make the difference between 40fps and 60fps. It will always reduce energy consumption. The same goes for unnecessary white space. The GPU has to work for each element it draws. Keeping all the elements at the smallest size possible without needing to compromise on the result will reduce the workload of the GPU. This is not primarily about disk storage, although you can potentially reduce it a lot (and even this matters when distributing skins). Confluence is by default a lean skin, suitable for low-end systems and IMO, it should be kept this way. Estuary is not suitable for a multitude of reasons. |
I just wanted to keep it working for myself and others. You obviously know a great deal about this stuff (and honestly in the time it took you to write that, you probably just could have fixed the textures in the way you're wanting!). But I don't really know enough about it to do it properly, nor do I have the time to do some big create / test loop to optimise things. My only goal was to get Confluence back to a state where it would work for me and my friends but I thought it might be useful for others, hence the PR. As it stands right now, subtitles and video calibration are completely broken, so unless Confluence is dead, someone needed to fix it for use with 20. I waitied a bit, no one did, so I thought at least for myself I will do what I can to get it working. And then popped this here as I thought it might help. As I said at the top there, I'm not a skinner and folks (Kodi team or otherwise) are free to take this if useful, or leave it, or just use it as a base to get started with a much better implementation. I am completely not fussed which. For my own uses, I have tested it on various (quit pathetic) older tablets, little odroids and other ARM machines, none of them in practise have any issue with this, that I can detect in actual use anyway - so I suspect the performance alarmism is a bit overblown, but again, you know way way more than I do which is why...I can't merge PRs, right? Please feel absolutely free to implement changes to make this better, indeed I encourage you to do so. Would love to see Confluence in safe hands and still receiving fixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be so kind and explain the "backdrop" element to me? I'm sure I'm missing something.
<viewtype label="100">icon</viewtype> | ||
<itemlayout height="50" width="60"> | ||
<control type="group"> | ||
<control type="image"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there even a point to this element? For me, it looks like that the following image control renders on top of this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why GH shows the previous lines. I mean the element at L83.
<focusedlayout height="50" width="60"> | ||
<control type="group"> | ||
<animation effect="zoom" time="200" tween="sine" easing="inout" start="100" end="124" center="auto">Focus</animation> | ||
<control type="image"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
I just did my best to follow: Take it up with them, I'd suggest. Again, Estuary does it like this, I just tried to grab the smallest bit I needed to make it work in Confluence. It's almost certain there is a better way if you understand skinning properly. Again, feel free to have at it. |
You are right, though - it seems to work fine without it, so have removed those elements and the texture. |
@bossanova808 Thanks much for your additions and your help. Much appreciated. 👍 |
Please could you help me, I have made the Xconfluence skin work correctly in kodi21, but the favorites do not work, I have downloaded confluence from here and favorites work, I have made a copy/paste of the MyFavourites file of the confluence skin to the DialogFavourites file of the skin Can I get favorites to work in the Xconfluence skin. they help me? thank you. |
Please ask in the Xonfluence thread on the forum. |
Please could you tell me what it is? |
Implement some of the Nexus related skin updtates/changes/needed fixes. I've gone for a 'get it working at least' approach as I am no skinner.
Video Calibration:
Totally broken by Nexus (in pretty much all skins bar the default). Added the needed reset control (pinching new texture from Estuary) - and fixed the mover controls. Also tweaked the existing subtitles texture to have 40px white above and below as per the recommendations. (See: https://forum.kodi.tv/showthread.php?tid=363553&pid=3133572#pid3133572)
New Window: Favourites Browser
Favourites has been promoted to a proper content window (
MyFavourites.xml
). I have implemented this in a minimal way.Replaces the old DialogFavourites.xml.
New control
colorbutton
and newDialogColorPicker.xml
(Fixes #191)I have implemented the control and the picker in a minimal but functional way.
Seekbar (Fixes #193)
Player.DisplayAfterSeek
has been removed, and the recommendation is to replace withPlayer.HasPerformedSeek(3)
(returns true for 3 seconds after a seek, vs. the old 2.5 seconds built-in, apparently. (See: https://forum.kodi.tv/showthread.php?tid=363553&pid=3098753#pid3098753)Skin Timers
Add a (currently empty)
Timers.xml
, to silence log warnings. Perhaps useful later?Hide Legacy CULyrics control (Fixes #173)
Simply set visibility to false as it solves the issue and I wasn't sure what to do with the various references to it in conditionals etc. without a bunch more work.
These fixes should probably go into master, and also a new branch for Nexus.
Of course you many not want them all, I'm really just using this PR as a place to collect the change set.