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
Implement TimeWarp Layers #778
Conversation
4e01a37
to
8541ba6
Compare
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.
First quick pass of comments. Going to fetch spends some time running the PR then will make a second pass of comments.
out.write("pref(\"gfx.compositor.override.clear-color.g\", \"0.0\");".getBytes()); | ||
out.write("pref(\"gfx.compositor.override.clear-color.b\", \"0.0\");".getBytes()); | ||
out.write("pref(\"gfx.compositor.override.clear-color.a\", \"0.0\");".getBytes()); | ||
} |
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.
You shouldn't need to set this. It should default to these values.
@@ -233,16 +258,18 @@ public void show(boolean focus) { | |||
} | |||
} | |||
|
|||
public void hide() { | |||
public void hide(boolean aRemove) { |
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.
Can this use and int instead of boolean? Something like hide(UIWidget.REMOVE_WIDGET);
and hide(UIWidget.KEEP_WIDGET);
. I found hide(true/false);
confusing.
final float rx = aX / mBrowserWidget.getTextureWidth(); | ||
final float ry = aY/ mBrowserWidget.getTextureHeight(); | ||
final float x = rx * (mBrowserWidget.getTextureWidth() - extra); | ||
final float y = ry * (mBrowserWidget.getTextureHeight() - extra); |
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.
Since the pixels are still the same size, I don't think you need to scale them? I think:
final int border = mBrowserWidget.getBorderWidth();
MotionEventGenerator.dispatch(widget, aDevice, aPressed, x - border, y - boarder);
Since we are just shifting the window by border
number of pixels. The compositor doesn't know there is a border so pixel size should be unaffected.
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.
does the GV motion event handle negative positions? x and y would be -1 in the 0,0 position
app/src/common/shared/org/mozilla/vrbrowser/ui/widgets/BrowserWidget.java
Show resolved
Hide resolved
app/src/common/shared/org/mozilla/vrbrowser/ui/widgets/MediaControlsWidget.java
Outdated
Show resolved
Hide resolved
@caseyyee layers require a transparent border. That means that we lost the border on the awesome bar in some situation. |
Hmm yeah, doesn't seem to be occuring for me after restart anymore. Will keep an eye out. Also looks like issue #754 no longer seems to be an issue. |
8541ba6
to
9783cb6
Compare
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.
Looks good. I wonder if there would be any value in the future to moving the layer code into VRB so that it can be part of the scenegraph? I didn't see any platform specific code in the VRLayers. Especially if we move sorting into the cull phase? not sure there is value. Just thinking out loud.
|
||
} else if (widget == mBrowserWidget && mBrowserWidget.getBorderWidth() > 0) { | ||
final int border = mBrowserWidget.getBorderWidth(); | ||
MotionEventGenerator.dispatch(widget, aDevice, aPressed, aX - border, aY - border); |
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.
Not sure what happened your comment. Negative number should be okay, but It's probably better to filter out any negative numbers since that would be outside the content anyway. But this is probably fine. If we find any issues I think we should address in followup.
@@ -908,14 +929,15 @@ void | |||
BrowserWorld::DrawWorld() { | |||
m.externalVR->SetCompositorEnabled(true); | |||
m.device->SetRenderMode(device::RenderMode::StandAlone); | |||
if (m.fadeAnimation) { | |||
m.fadeAnimation->UpdateAnimation(); |
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.
m.fadeAnimation seems like a good candidate for vrb::Updatable, that way you can use RenderContext::GetFrameDelta() to make the animations time based. If you agree, just file as a follow up.
@@ -38,6 +38,11 @@ struct EyeRect { | |||
mHeight = aRect.mHeight; | |||
return* this; | |||
} | |||
|
|||
bool IsDefault() const { |
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.
This function doesn't seem to be used anywhere?
* This glitch doesn't happen with normal scenegraph code because the background color is shown | ||
* on the back of the Quad surface. But with Layers we can't do that. | ||
*/ | ||
return; |
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 I understand the issue but it sounds like we were drawing a blank quad while resizing. I'm surprised this didn't cover the layer since the content layer is drawn first? Feel free to file a follow up if there is still an out standing issue.
#include <memory> | ||
#include <functional> | ||
#include <jni.h> | ||
|
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.
Looks like functional and jni.h are left overs here to? I didn't see then being used any where.
namespace crow { | ||
|
||
static ovrMatrix4f ovrMatrixFrom(const vrb::Matrix& aMatrix) { |
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.
This looks like a good candidate for a member function in vrb::Matrix. Something like CopyTransposed(float[4][4] aTarget)
? As a follow up if you agree.
Woohoo! Thanks guys :) |
No description provided.