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

Stereoscopic depth for Confluence #8147

Merged
merged 2 commits into from Oct 11, 2015
Merged

Conversation

da-anda
Copy link
Member

@da-anda da-anda commented Sep 27, 2015

This PR adds subtile stereoscopic depth to Confluence. OSD and dialogues will pop out of the screen so that they stay on top of random objects from videos that also pop out. The popout effect is using ~60% of the possible depth which should be enough for everage movie scenes. It won't cover all cases ofc, but it'll be way to extreme if we'd go all the way for main UI elements. The main content area stays at level 0, which means that while you're browsing in GUI your eye can focus directly to the screen without hurting your eyes. The downside of this is, that a potentially playing movie in background will cause distractions on transparent areas. To compensate for that I pushed back the video background panel, but this doesn't seem to have an effect atm. Maybe @afedchin could have a look.

This PR depends on #8146 as it uses constants for the depths values for easy tweaking.

@ronie @phil65 @BigNoid please review. To simplify some things I created an include for the common global header (breadcrumb + clock) - not sure you like that, but I hope so.

<include name="CommonWindowHeader">
<param name="icon" value="icon_addons" />
<param name="label" value="$LOCALIZE[24001]" />
</include

This comment was marked as spam.

@BigNoid
Copy link
Member

BigNoid commented Sep 27, 2015

@da-anda We should use CamelCase for param names (see #6807 for reference). I dont have 3D tv so cant test the functionality.

@@ -8,6 +8,7 @@
</coordinates>
<controls>
<control type="group">
<depth>DepthDialogue+</depth>

This comment was marked as spam.

@da-anda
Copy link
Member Author

da-anda commented Sep 28, 2015

I addressed the comments so far and fixed some other things I noticed (gave dialogs that usually show up on top of others a higher depth, like DialogOK).
@BigNoid I was using red/cyan mode to do this - so also didn't have a 3D TV at hand to test. That said, most things should be fine, but it's quite possible that certain depth values might need a little tweaking once this is tested by users on TVs

<include>Window_OpenClose_Animation</include>
<animation effect="slide" start="0,0" end="-40,0" time="75" condition="Window.IsVisible(Mutebug)">conditional</animation>
</control>
</depth>

This comment was marked as spam.

This comment was marked as spam.

@da-anda
Copy link
Member Author

da-anda commented Oct 4, 2015

updated. Should hopefully be ready for prime time now. Things I changed since last update:

  • overstretched background images a bit to prevent an ugly bleeding effect in 3D mode (due to the parallax shift we ended up with black pixels and did not fill the entire canvas)
  • stretched some other background graphics to prevent some bleeding on edges (underlying layers where visible while they should have been hidden/covered by the background of the dialog/sideblade etc)
  • added a depth to items in the footer (now playing + pagination)
  • fixed depth of clock in info dialogs
  • changed fanart in visualisation view (fullscreen playback) to keep aspect ratio (just like background fanart in library views)

Skinners, please give it a short review again and hit the button if you like

@ronie
Copy link
Member

ronie commented Oct 4, 2015

overstretched background images a bit to prevent an ugly bleeding effect in 3D mode (due to the parallax shift we ended up with black pixels and did not fill the entire canvas)
stretched some other background graphics to prevent some bleeding on edges (underlying layers where visible while they should have been hidden/covered by the background of the dialog/sideblade etc)

is this something that can (or maybe should) be fixed in core?
it doesn't feel right to alter the skin like that to workaround limitations we may have in core.
especially if it's for a feature that only a fraction of our users are using, but will affect all users.
also it makes skinning rather awkward if we have to start coding from position (-10,-10) and use a 1300x731 resolution.

other than that, i'm fine with adding 3D support to Confluence.
earlier today, before your latest adjustments, i've tested this PR on my 3D TV.
as far as i could tell you've got all the depth tags correct, i didn't spot any elements or dialogs that appeared to be out of place. it certainly adds a nice touch to the skin, so thx!

@da-anda
Copy link
Member Author

da-anda commented Oct 5, 2015

thanks for review and testing @ronie. I also thought that it would be nicer to handle this in core, but I honestly have no idea if this will work reliable. Skinning engine does not know which control is supposed to be used as a background and therefor could be overstretched. We could ofc check if the control touches the edges of the canvas and then shift it for the exactly required 3D offset and adjust the width accordingly, but this might also effect controls with text or lead to other alignment oddities. We can ofc give it a try.

Also, the 1300x731 offset is not set to stone, it's a random value I found to work well with our used 3D depths of these items, so if a skin would use -1 as depth, it might need more overstretching. And I just realized that I didn't need to change the hight as long as the picture aspect ratio is set correctly for the control.

How would you like to go on from here, put this PR on hold and try to find a core solution or merge it for now and then try to find a core solution?

@phil65
Copy link
Contributor

phil65 commented Oct 5, 2015

There is a system setting for this 3D effect, right? What about putting the image dimensions into includes so we take the slightly oversized fanart dimensions when 3D effect is active and the normal image dimensions when no 3d effect is active?
I doubt that we can solve this in core.

@da-anda
Copy link
Member Author

da-anda commented Oct 5, 2015

mind giving an example of what you had in mind?

@phil65
Copy link
Contributor

phil65 commented Oct 5, 2015

Of course.

<control type="image">
<include condition="system.getbool(settings.name)">include_with_oversized_dims</include>
<include condition="!system.getbool(settings.name)">include_with_normal_dims</include>
...
</control>

@ronie
Copy link
Member

ronie commented Oct 5, 2015

instead of includes, maybe use a conditional zoom animation based on that setting?

@phil65
Copy link
Contributor

phil65 commented Oct 5, 2015

Would also be fine, yes.

@BigNoid
Copy link
Member

BigNoid commented Oct 5, 2015

Whats the advantage of zoom animations instead of includes? The zoom animation is evaluated at every frame afaik, while the include is only evaluated at window load. Not sure if it has any impact in this case, but in theory the zoom animations are less efficient.

@phil65
Copy link
Contributor

phil65 commented Oct 5, 2015

True, but the impact is probably negligible and animations seem to look a bit cleaner to me in skin code. (just one additional line for every bg image control + no need for two additional includes) I dont mind which one we take, but i think it would be nice to avoid scaling for non-3d setups by using one of these methods.

@BigNoid
Copy link
Member

BigNoid commented Oct 5, 2015

In this case cleaner code is better indeed. Sorry for noise.

@da-anda
Copy link
Member Author

da-anda commented Oct 9, 2015

I moved the background dimension and zoom stuff to an include and used that all over. Feel free to review and push the button

@da-anda
Copy link
Member Author

da-anda commented Oct 11, 2015

ok guys, I realized we really need animation support for the depth because adding it to <focusedlayout> is just not gonna work. It's sort of ok-ish in Confluence but just not gonna work in other skins. Issue is that most zoom effects for which stereoscopic effects make sense for are done via conditional animations on top of <focusedlayout>, as there is a difference between "last focused item" (which the tag actually is used for) and "actively focused", which is triggered via conditional animations.

Anybody able to implement it into our GUI engine (will require handling of z coordinates correctly, so not a simple fix from a first look).
@afedchin maybe? @jmarshallnz are you still familiar with that code so that you could give a helping hand on what needs to be done to implement it?

da-anda added a commit that referenced this pull request Oct 11, 2015
Stereoscopic depth for Confluence
@da-anda da-anda merged commit 1b13735 into xbmc:master Oct 11, 2015
@da-anda da-anda deleted the confluence-3D branch October 11, 2015 16:22
@jmarshallnz
Copy link
Contributor

  1. Figure out what coordinate system you want to be in. i.e. how large should Z be? You can do this by figuring out what a rotate around the X or Y axis should look like. It's probably related to how the camera stuff works atm (x+y are set for that - can't recall if z is, but z is what you want to be able to define - how far the camera is away). We currently allow moving the camera (x/y?) so that you can still get stuff square to the camera easily. I think you'll still need that. I think you'll want much larger z values than you currently have in m_stereo - similar to x/y I think.
  2. Get rid of the SetStereoFactor stuff. Instead, have a z in the control (i.e. your m_stereo currently).
  3. Pass z down into the GUITexture and GUIFont renderers.
  4. You'll probably want the CGUIControlGroup::SetOrigin() stuff to pass their z along when they offset the control locations (thus will need a 3 point structure for GraphicContext::m_origins)

I'd start with 2-4 and see what happens. Biggest issue IMO will be playing around with camera distance to get it looking reasonable. By the looks that's what your current stereo factor stuff in graphiccontext is doing - looks like that can be dropped and a fixed cam location used.

@da-anda
Copy link
Member Author

da-anda commented Oct 12, 2015

thanks @jmarshallnz - I had the feeling that this requires bigger changes from a quick look. Let's see if I can get my head around it.

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.

None yet

8 participants