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.

Sign up for GitHub

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

Jump to bottom

Feature/#429 add missing video props #505

Merged
merged 15 commits into from
Mar 30, 2020
Merged

Feature/#429 add missing video props #505

merged 15 commits into from
Mar 30, 2020

Conversation

panwrona
Copy link
Contributor

Add timedTextPath and seekTo properties. Click link below to see how it looks like on the video:

Resolve #429

Video

darek607
darek607 reviewed Mar 26, 2020
View reviewed changes
android/src/main/java/com/magicleap/magicscript/scene/nodes/video/VideoPlayerImpl.kt Outdated
Comment on lines 95 to 98
prepareMediaPlayer()
} else {
addTimedTextPath(subtitlesPath.toString(), onSubtitleChangeListener)
prepareMediaPlayer()
Copy link
Contributor

Choose a reason for hiding this comment

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

preparareMediaPlayer() can be moved to line 85

android/src/main/java/com/magicleap/magicscript/scene/nodes/video/VideoNode.kt Outdated
Comment on lines 118 to 127
UiTextNode.PROP_TEXT_SIZE,
0.1,
PROP_LOCAL_POSITION,
JavaOnlyArray.of(localPosition.x, localPosition.y, localPosition.z + 0.01f),
PROP_LOCAL_SCALE,
JavaOnlyArray.of(localScale.x, localScale.y, localScale.z),
PROP_ALIGNMENT,
"top-center",
PROP_BOUNDS_SIZE,
JavaOnlyMap.of(PROP_BOUNDS_SIZE, JavaOnlyArray.of(getBounding().size().x, getBounding().size().y)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we somehow refactor this? Like:

UiTextNode.PROP_TEXT_SIZE, 0.1,
PROP_LOCAL_POSITION, JavaOnlyArray.of(localPosition.x, localPosition.y, localPosition.z + 0.01f)
...

android/src/main/java/com/magicleap/magicscript/scene/nodes/video/VideoNode.kt Outdated
private fun createSubtitles() {
if (subtitles == null) {
subtitles = UiSubtitlesNode(
props = getSubtitleProps(), context = context, viewRenderableLoader = viewRenderableLoader, nodeClipper = nodeClipper, fontProvider = fontProvider)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is probably too long, lets put each property in a new line. Also the closing bracket should be in the new line ;-)

android/src/main/java/com/magicleap/magicscript/scene/nodes/views/OutlineTextView.kt
@@ -0,0 +1,54 @@
package com.magicleap.magicscript.scene.nodes.views
Copy link
Contributor

Choose a reason for hiding this comment

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

licence

android/src/main/java/com/magicleap/magicscript/scene/nodes/video/SubtitlesClient.kt Outdated
@@ -0,0 +1,63 @@
package com.magicleap.magicscript.scene.nodes.video
Copy link
Contributor

Choose a reason for hiding this comment

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

licence

android/src/main/java/com/magicleap/magicscript/scene/nodes/video/UiSubtitlesNode.kt
@@ -0,0 +1,33 @@
package com.magicleap.magicscript.scene.nodes.video
Copy link
Contributor

Choose a reason for hiding this comment

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

licence

android/src/main/java/com/magicleap/magicscript/scene/nodes/video/VideoNode.kt Outdated
@@ -91,6 +106,38 @@ class VideoNode(
setAction(props)
setLooping(props)
setVolume(props)
setSeekTo(props)
if(props.containsAny(PROP_SIZE, PROP_LOCAL_SCALE, PROP_LOCAL_POSITION)) updateSubtitlesProps()
Copy link
Contributor

Choose a reason for hiding this comment

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

Use brackets after the if and put updateSubtitlesProps() into new line (we followed this practice so far)

android/src/main/java/com/magicleap/magicscript/scene/nodes/video/VideoNode.kt Outdated
Comment on lines 182 to 159
if(subtitles != null) {
contentNode.removeChild(subtitles)
subtitles = null
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put this after super.onDestroy?

android/src/main/res/layout/subtitles.xml Outdated
@@ -0,0 +1,27 @@
<?xml version="1.0" encoding="utf-8"?>
<!--
~ Copyright (c) 2019 Magic Leap, Inc. All Rights Reserved
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be used new licence: 2019-2020 according to this PR

@darek607
Copy link
Contributor

darek607 commented Mar 26, 2020 •
edited
Loading

I tested the Video scene from the Catalog and the subtitles are not positioned correctly (the position depends on movie). Hint: in the getSubtitleProps() you should probably return contentNode.localScale instead localScale), because video node size is actually achieved be scaling the content node - this is because the ExternalTexture is always 1m x 1m). See the comment in VideoNode line 86.

subtitles_too_high

subtitles_out_of_video

This was referenced Mar 26, 2020
Open
Closed
@darek607
Copy link
Contributor

darek607 commented Mar 27, 2020 •
edited
Loading

Another issues discovered when testing:

  1. Subtitles work only first time: when I replay the video (stop -> start), the subtitles do not change (always the last text is displaying)

  2. Subtitles do not display in release build.

  3. Trying to play local movie when USB is disconnected (debug mode) results in app crash. We should probably handle this, because before subtitles support was added, the video did not play in such case, but app was not crashing.
    Here is the log:

E/AndroidRuntime: FATAL EXCEPTION: OkHttp Dispatcher
    Process: com.catalog, PID: 15727
    java.lang.Error: java.net.ConnectException: Failed to connect to localhost/127.0.0.1:8081
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1168)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:636)
        at java.lang.Thread.run(Thread.java:764)

kpiascik
kpiascik reviewed Mar 27, 2020
View reviewed changes
android/src/main/java/com/magicleap/magicscript/scene/nodes/video/UiSubtitlesNode.kt Outdated
@@ -1,3 +1,19 @@
/*
* Copyright (c) 2019-2020 Magic Leap, Inc. All Rights Reserved
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is a new file then it shouldn't have 2019 since it was created in 2020.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@darek607 darek607 self-requested a review March 27, 2020 13:16
@panwrona panwrona force-pushed the feature/#429_add_missing_video_props branch from 379a481 to a34a489 Compare March 30, 2020 09:41
@panwrona panwrona merged commit 17f79a0 into master Mar 30, 2020
@panwrona panwrona deleted the feature/#429_add_missing_video_props branch March 30, 2020 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darek607 darek607 darek607 approved these changes

@kpiascik kpiascik kpiascik approved these changes

@grzegorzdec grzegorzdec Awaiting requested review from grzegorzdec

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update [WebView] default properties
3 participants
@panwrona @darek607 @kpiascik