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
Add Windows Mixed Reality support #85
Conversation
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 added a few (personal) thoughts. I'll let @xen2 review it properly as I'm no expert in that area.
/// </summary> | ||
/// <param name="value">The value.</param> | ||
/// <returns>The result of the conversion.</returns> | ||
public static implicit operator Matrix(System.Numerics.Matrix4x4 value) |
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 a big fan of having the conversion implicit in both way. Usually one is implicit and the other explicit.
@xen2 What is the impact of having dependency to System.Numerics
? Shouldn't that be handled more generally (including testing) in a separate issue?
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.
@Kryptos-FR As Windows.Perception.Spatial uses System.Numerics types I needed to add these implicit conversions to reduce bloat. There should be nothing wrong with making the conversion implicit both ways as both use floats. I mean, even the existing conversions are implicit both ways if both types use the same underlying primitive types.
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.
@Kryptos-FR It seems like System.Numerics
is already an implicit dependency (quite surprised, not sure where it comes from, seems automatic for Windows and UWP platforms!)
@Aminator9000 Not a big fan of having things halfway (conversion only on UWP).
So either we:
- Add
System.Numerics
explicitly (NuGet version) and this conversion for all platforms. It implies you are able to tests those platforms (at least compilation with theXenko.*.sln
) - You move this
System.Numerics
implicit conversion temporarily where you need it as an explicit method (and we can always add it back to Mathematics later for everybody)
/// </summary> | ||
/// <param name="value">The value.</param> | ||
/// <returns>The result of the conversion.</returns> | ||
public static implicit operator Quaternion(System.Numerics.Quaternion value) |
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 remark about both-way implicit conversion.
/// </summary> | ||
/// <param name="value">The value.</param> | ||
/// <returns>The result of the conversion.</returns> | ||
public static implicit operator Vector3(System.Numerics.Vector3 value) |
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 remark about both-way implicit conversion.
@@ -116,7 +116,7 @@ public partial class ForwardRenderer : SceneRendererBase, ISharedRenderer | |||
[DefaultValue(true)] | |||
public bool BindDepthAsResourceDuringTransparentRendering { get; set; } = true; | |||
|
|||
private Logger logger { get; } = GlobalLogger.GetLogger(nameof(ForwardRenderer)); | |||
private Logger Logger { get; } = GlobalLogger.GetLogger(nameof(ForwardRenderer)); |
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 was about to say that private
fields are always lowercase. But then I realized it is actually a private
property.
I don't see the point of having a private
property here. I think it should be converted to a private readonly
field.
|
||
namespace Xenko.Graphics | ||
{ | ||
public class WindowsMixedRealityGraphicsPresenter : GraphicsPresenter |
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.
Having that class in Xenko.Graphics
forces to have dependency on Windows.Graphics.Holographic
for all UWP projects (even those that are not targeting such device. I think we should come up with a better design.
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.
Yes and ideally it should all be doable as a plugin, but there is unfortunately quite a bunch of non-trivial extensions point to add, could be a lot of work (it affects renderer code and game creation).
Maybe we'll have to keep it together for now (just like we do for all platforms currently) and consider splitting it later down the road.
} | ||
|
||
public bool IsWindowMixedReality { get; } |
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.
Abstraction leak?
Will start testing this. Good work :) |
backBuffer = new Texture(GraphicsDevice).InitializeFromImpl(d3DBackBuffer, false); | ||
|
||
LeftEyeBuffer = backBuffer.ToTextureView(new TextureViewDescription() { ArraySlice = 0, Type = ViewType.Single }); | ||
RightEyeBuffer = backBuffer.ToTextureView(new TextureViewDescription() { ArraySlice = 1, Type = ViewType.Single }); |
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.
Those two buffers should be disposed if they exist before being recreated.
@@ -0,0 +1,202 @@ | |||
// Copyright (c) Xenko contributors (https://xenko.com) and Silicon Studio Corp. (https://www.siliconstudio.co.jp) |
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.
New files should only have Copyright for Xenko contributors, not Silicon Studio anymore
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.
@xen2 Let's add something in the doc/readme or somewhere else (wiki?) to precise a template for new files.
I think we could also port the "coding guidelines" we had in confluence and ad it to the wiki, to ensure a uniform coding style and naming conventions.
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.
@Kryptos-FR Yes we should add that. The new version (with Xenko contributors only) was already in DotSettings, but
- not everybody has resharper
- people might edit files through another solution (we have a DotSettings only for
Xenko.sln
but maybe it was done throughXenko.UWP.sln
in this case)
@@ -0,0 +1,172 @@ | |||
// Copyright (c) Xenko contributors (https://xenko.com) and Silicon Studio Corp. (https://www.siliconstudio.co.jp) |
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.
New files should only have Copyright for Xenko contributors, not Silicon Studio anymore
/// </summary> | ||
/// <param name="value">The value.</param> | ||
/// <returns>The result of the conversion.</returns> | ||
public static implicit operator Matrix(System.Numerics.Matrix4x4 value) |
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.
@Kryptos-FR It seems like System.Numerics
is already an implicit dependency (quite surprised, not sure where it comes from, seems automatic for Windows and UWP platforms!)
@Aminator9000 Not a big fan of having things halfway (conversion only on UWP).
So either we:
- Add
System.Numerics
explicitly (NuGet version) and this conversion for all platforms. It implies you are able to tests those platforms (at least compilation with theXenko.*.sln
) - You move this
System.Numerics
implicit conversion temporarily where you need it as an explicit method (and we can always add it back to Mathematics later for everybody)
@Aminator9000 Please let me know when this is ready for final review/merge (if not already). |
@xen2 It is ready now although the above mentioned issues are still there. Those could be fixed later as I am currently not sure how to fix them. |
Sorry for the late review, LGTM! |
Please, help me to create a minimal WMR application. I do all what neccessary after reading all about VR in docs. I create Game from template with UWP support, remove XAML Page component and edit protected override void OnLaunched(LaunchActivatedEventArgs e)
{
Game = new Game();
Game.UnhandledException += Game_UnhandledException;
Game.Run(new GameContextUWPCoreWindow(null, 0, 0, true));
} WMR portal started, VR controllers visible in simulator, but all around is black. What I do wrong? |
I found solution - *.t4 in Xenko sources contain code to create |
In addition to Oculus and OpenVR I added Windows Mixed Reality support for UWP. This brings not only support for WMR headsets (which were already supported thanks to SteamVR) but also HoloLens, although testing for the latter would be needed. As the WMR motion controllers have a thumbstick and a touchpad, I made it available to differentiate between the two while they still map to the same on the other platforms.
To use Windows Mixed Reality you just add the API to the list of required APIs and preferably set it at the top of the list so that it won't use OpenVR which would result in an exception. Next you need to start the game from a CoreWindow context and set the optional parameter "isWindowsMixedReality" to true or else it uses the swap chain of the CoreWindow instead of the holographic space. Be sure to not activate the CoreWindow as this must happen after creating the holographic space which is done internally.
Current issues:
I hope this addition will lead to more titles produced with WMR support and as always please test it if you have a WMR device (or emulate it with the Windows Mixed Reality Portal app).