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

Adding GazeIntegration #2026

Merged
merged 171 commits into from
Apr 30, 2018
Merged

Adding GazeIntegration #2026

merged 171 commits into from
Apr 30, 2018

Conversation

harishsk
Copy link
Contributor

@harishsk harishsk commented Apr 26, 2018

Issue: #

PR Type

What kind of change does this PR introduce?

Feature

What is the current behavior?

Not applicable

What is the new behavior?

This pull request adds a new library (Microsoft.Toolkit.Uwp.Input.GazeInteraction) for supporting gaze interaction in UWP applications. The library is built on top of RS4 API for eye gaze and makes it easy to add eye gaze interactions to existing and new UWP applications.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Docs have been added/updated which fit documentation template. (for bug fixes / features)
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

harishsk and others added 30 commits April 9, 2018 11:10
…t/UWPCommunityToolkit into harishsk/gaze-integration
…ion" and "origin/harishsk/gaze-integration"
…ke behaviour, not GazePointer, and that the invokability should be cached within them.
<files>
<!-- WinMd and IntelliSense files -->
<file src="..\Microsoft.Toolkit.Uwp.Input.GazeInteraction\Output\Win32\Release\Microsoft.Toolkit.Uwp.Input.GazeInteraction.winmd" target="lib\uap10.0"/>
<file src="..\Microsoft.Toolkit.Uwp.Input.GazeInteraction\Output\Win32\Release\Microsoft.Toolkit.Uwp.Input.GazeInteraction.xml" target="lib\uap10.0"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be \lib\uap10.0.17134\
@onovotny ?


<!-- DLLs and resources -->
<file src="..\Microsoft.Toolkit.Uwp.Input.GazeInteraction\Output\ARM\Release\Microsoft.Toolkit.Uwp.Input.GazeInteraction.dll" target="runtimes\win10-arm\native"/>
<file src="..\Microsoft.Toolkit.Uwp.Input.GazeInteraction\Output\ARM\Release\Microsoft.Toolkit.Uwp.Input.GazeInteraction.pri" target="runtimes\win10-arm\native"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to put just a single .pri file in the \lib\ folder next to the winmd file

<Implementation>Microsoft.Toolkit.Uwp.Input.GazeInteraction.dll</Implementation>
</Reference>
<ReferenceCopyLocalPaths Include="$(MSBuildThisFileDirectory)..\..\runtimes\win10-$(Lib-Platform)\native\Microsoft.Toolkit.Uwp.Input.GazeInteraction.dll" />
</ItemGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

None of this should be necessary. However this should be added:

 <ItemGroup Condition="'$(ProjectExt)' != '.vcxproj'">
    <SDKReference Include="Microsoft.VCLibs, Version=14.0">
      <Name>Visual C++ 2015 Runtime for Universal Windows Platform Apps</Name>
    </SDKReference>
  </ItemGroup>

<file src="..\Microsoft.Toolkit.Uwp.Input.GazeInteraction\Output\Win32\Release\Microsoft.Toolkit.Uwp.Input.GazeInteraction.dll" target="runtimes\win10-x86\native"/>
<file src="..\Microsoft.Toolkit.Uwp.Input.GazeInteraction\Output\Win32\Release\Microsoft.Toolkit.Uwp.Input.GazeInteraction.pri" target="runtimes\win10-x86\native"/>

<file src="..\Microsoft.Toolkit.Uwp.Input.GazeInteraction\Microsoft.Toolkit.Uwp.Input.GazeInteraction.targets" target="build\native"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be in \build\uap10.0.17134\ ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just updated the nuspec and targets, tested it, and seems to work as expected.

@dotMorten
Copy link
Contributor

dotMorten commented Apr 30, 2018

Updated OMD. This looks great now.
image
image


BEGIN_NAMESPACE_GAZE_INPUT

private ref class GazeStats sealed
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this was changed to private, but I don't see the class referenced anywhere. Can we just delete it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will be very shortly (immediately after this release) adding stuff that needs this. Is it okay to leave it in till then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Just checking :)


// Basic filter which performs no input filtering -- easy to
// use as a default filter.
public ref class NullFilter sealed : public IGazeFilter
Copy link
Contributor

Choose a reason for hiding this comment

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

All the other filters are internal. Should this be too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, Thanks for catching it. I will submit a fix soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

That means IGazeFilter and GazeFilterArgs are not use anywhere else either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and submitted. I also fixed the PointerState enum with explicit values. Thanks for the careful review.
Please merge if everything else is good. Thanks.

Copy link
Contributor

@dotMorten dotMorten left a comment

Choose a reason for hiding this comment

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

Looks good, but 3 things that might need addressing:

  1. NullFilter is public. All other filters aren't.
  2. There's an enum where the order is important, so it should probably get values explicitly assigned to each
  3. GazeStats looks like an unused private class (isn't used anywhere outside itself)

@dotMorten
Copy link
Contributor

One more thing: Since NullFilter is becoming internal, that means IGazeFilter and GazeFilterArgs are not used anywhere publicly, and can be internalized as well.

…and added explicit values to PointerState enum
@nmetulev nmetulev merged commit 9b40a2f into rel/3.0.0-preview Apr 30, 2018
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.

5 participants