Feature/#429 add missing video props by panwrona · Pull Request #505 · magic-script/magic-script-components-react-native

panwrona · 2020-03-26T12:15:33Z

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

Resolve #429

darek607 · 2020-03-26T12:58:30Z

+            prepareMediaPlayer()
+        } else {
+            addTimedTextPath(subtitlesPath.toString(), onSubtitleChangeListener)
+            prepareMediaPlayer()


preparareMediaPlayer() can be moved to line 85

darek607 · 2020-03-26T13:15:28Z

+            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)))


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) ...

darek607 · 2020-03-26T13:17:11Z

+    private fun createSubtitles() {
+        if (subtitles == null) {
+            subtitles = UiSubtitlesNode(
+                    props = getSubtitleProps(), context = context, viewRenderableLoader = viewRenderableLoader, nodeClipper = nodeClipper, fontProvider = fontProvider)


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

darek607 · 2020-03-26T13:20:00Z

@@ -0,0 +1,54 @@
+package com.magicleap.magicscript.scene.nodes.views


darek607 · 2020-03-26T13:20:27Z

@@ -0,0 +1,63 @@
+package com.magicleap.magicscript.scene.nodes.video


darek607 · 2020-03-26T13:20:47Z

@@ -0,0 +1,33 @@
+package com.magicleap.magicscript.scene.nodes.video


darek607 · 2020-03-26T13:22:44Z

        setLooping(props)
        setVolume(props)
+        setSeekTo(props)
+        if(props.containsAny(PROP_SIZE, PROP_LOCAL_SCALE, PROP_LOCAL_POSITION)) updateSubtitlesProps()


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

darek607 · 2020-03-26T13:25:16Z

+        if(subtitles != null) {
+            contentNode.removeChild(subtitles)
+            subtitles = null
+        }


Can we put this after super.onDestroy?

darek607 · 2020-03-26T13:28:29Z

@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!--
+  ~ Copyright (c) 2019 Magic Leap, Inc. All Rights Reserved


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

darek607 · 2020-03-26T14:58:57Z

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.

darek607 · 2020-03-27T07:29:42Z

Another issues discovered when testing:

Subtitles work only first time: when I replay the video (stop -> start), the subtitles do not change (always the last text is displaying)
Subtitles do not display in release build.
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 · 2020-03-27T13:07:35Z

@@ -1,3 +1,19 @@
+/*
+ * Copyright (c) 2019-2020 Magic Leap, Inc. All Rights Reserved


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

panwrona requested review from darek607 and grzegorzdec March 26, 2020 12:15

darek607 reviewed Mar 26, 2020

View reviewed changes

This was referenced Mar 26, 2020

Update [WebView] default properties #429

Open

Implement [Video] properties #428

Closed

kpiascik reviewed Mar 27, 2020

View reviewed changes

darek607 self-requested a review March 27, 2020 13:16

darek607 approved these changes Mar 27, 2020

View reviewed changes

kpiascik approved these changes Mar 27, 2020

View reviewed changes

panwrona and others added 13 commits March 30, 2020 11:37

Add seekTo and timedTextPath properties to VideoNode

c41c0fe

Add timedTextPath and seekTo option

fd76f2e

Add selecting track to media player

d5e5a05

Add versioning for video player

0c41d66

Add UiSubtitlesNode

e6a9478

Add another logs

8b782ec

Add video node subtitles

7bf9de2

Improve video node subtitles changing and showing

0317a13

Remove unnecessary code

ca49144

Remove subtitle client and use UriFileProvider to obtain subtitles

00e9436

subtitles positioning fix

1e2de18

Add missing licenses

7ea3026

Updated file provider error log and licenses

a34a489

panwrona force-pushed the feature/#429_add_missing_video_props branch from 379a481 to a34a489 Compare March 30, 2020 09:41

panwrona added 2 commits March 30, 2020 12:05

Fix tests

fa4e9ad

Fix tests

39296a3

panwrona merged commit 17f79a0 into master Mar 30, 2020

panwrona deleted the feature/#429_add_missing_video_props branch March 30, 2020 12:02

Navigation Menu

Uh oh!

Feature/#429 add missing video props - #505

Feature/#429 add missing video props#505
panwrona merged 15 commits into
masterfrom
feature/#429_add_missing_video_props

panwrona commented Mar 26, 2020

Uh oh!

darek607 Mar 26, 2020

Uh oh!

darek607 Mar 26, 2020

Uh oh!

darek607 Mar 26, 2020

Uh oh!

darek607 Mar 26, 2020

Uh oh!

darek607 Mar 26, 2020

Uh oh!

darek607 Mar 26, 2020

Uh oh!

darek607 Mar 26, 2020

Uh oh!

darek607 Mar 26, 2020

Uh oh!

darek607 Mar 26, 2020

Uh oh!

darek607 commented Mar 26, 2020 •

edited

Loading

Uh oh!

darek607 commented Mar 27, 2020 •

edited

Loading

Uh oh!

kpiascik Mar 27, 2020

Uh oh!

panwrona Mar 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -0,0 +1,54 @@
		package com.magicleap.magicscript.scene.nodes.views

		@@ -0,0 +1,63 @@
		package com.magicleap.magicscript.scene.nodes.video

		@@ -0,0 +1,33 @@
		package com.magicleap.magicscript.scene.nodes.video

		@@ -1,3 +1,19 @@
		/*
		* Copyright (c) 2019-2020 Magic Leap, Inc. All Rights Reserved

Uh oh!

Conversation

panwrona commented Mar 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darek607 commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darek607 commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

darek607 commented Mar 26, 2020 •

edited

Loading

darek607 commented Mar 27, 2020 •

edited

Loading