feat(PanoViewer): improve cubemap support #131

happyhj · 2018-01-22T09:12:31Z

Issue

#123

Details

feat

update examples for new image api
remove projectionType "vertical_cubestrip"
add projectionType "cubemap"
added tile config options to support various cube map layouts

fix

set preserveDrawingBuffer false only where the tremor occurs

test

increate mocha test timeout

chore

change script version to "latest" on demo site

I removed code for support old mobile browsers with "vertical_cubestrip" projection, which is removed from this PR. But we should still support for those browsers.
We will continue to work on the issues below.
#132

* increate mocha test timeout * change script version to "latest" on demo site * update examples for new image api * remove projectionType "vertical_cubestrip" * add projectionType "cubemap" * set preserveDrawingBuffer false only where the tremor occurs * added tile config options to support various cube map layouts ref #123

jongmoon · 2018-01-23T02:04:24Z

mocha.opts

@@ -1 +1 @@
--timeout 10000


@happyhj Why change?

Thanks. I found we don't need to expand the timeout. I will revert that.

jongmoon · 2018-01-23T02:17:50Z

src/PanoImageRenderer/PanoImageRenderer.js

@@ -455,7 +480,7 @@ export default class PanoImageRenderer extends Component {
 		gl.uniformMatrix4fv(this.shaderProgram.mvMatrixUniform, false, this.mvMatrix);

 		if (this._isVideo) {
-			this._renderer.texImage2D(this.context, this._image);
+			this._bindTexture();


@happyhj Should it be bound again?

I got it. I'll change the flow to prevent duplicated binding texture.

jongmoon · 2018-01-23T02:52:37Z

src/PanoImageRenderer/PanoImageRenderer.js

@@ -112,14 +114,34 @@ export default class PanoImageRenderer extends Component {
 			.catch(e => setTimeout(() => { throw e; }));// Prevent exceptions from being isolated in promise chain.
 	}

+	static extractImageSource(imgParam) {


@happyhj Isn't this job already done in contentLoader?

Thanks, I will remove those methods.

jongmoon · 2018-01-23T07:51:10Z

src/PanoImageRenderer/renderer/CubeRenderer.js


 		try {
-			gl.pixelStorei(gl.UNPACK_FLIP_Y_WEBGL, false);
 			gl.bindTexture(gl.TEXTURE_CUBE_MAP, texture);



The renderer doesn't need to flip the texture anymore. Now we use texture coordinate like the sphere renderer.

jongmoon · 2018-01-23T07:52:29Z

src/PanoImageRenderer/renderer/CubeRenderer.js

 			gl.bindTexture(gl.TEXTURE_CUBE_MAP, texture);

-			this.texImage2D(gl, image);
+			if (image instanceof Array) {


It would be better to separate following codes as a texImage2D. Because it is not needed to call bindTexture on every frame.

jongmoon · 2018-01-23T08:06:50Z

demo/examples/cubemap_video.html

+        container,
+        {
+            projectionType: "cubemap",
+            video: {


@happyhj I didn't catch before... I feel it makes confusion... because video param tend to have too many types. How about to separate a cubemapConfig

thank you for feedback. I will add cubemapConfig option as you said.

jongmoon · 2018-01-26T01:58:31Z

src/PanoImageRenderer/renderer/SphereRenderer.js

@@ -50,7 +50,7 @@ export default class SphereRenderer extends Renderer {
 			}`;
 	}

-	static bindTexture(gl, texture, image) {
+	static updateTexture(gl, texture, image) {
 		if (!image) {


updateTexture in SphereRenderer only need to call this.texImage2D and size check logic need to be moved to bindTexture

thanks, I'll apply that.

jongmoon · 2018-01-26T02:06:17Z

src/PanoImageRenderer/PanoImageRenderer.js

+			this._image,
+			this._imageConfig,
+		);
+		this._shouldForceDraw = true;


shouldForceDraw=true is not valid. because when user pause the video, it should not draw a frame.

Ok, I'll remove it.

jongmoon · 2018-01-26T02:11:25Z

test/unit/PanoViewer/PanoViewer.spec.js

@@ -245,6 +245,24 @@ describe("PanoViewer", function() {
 		});
 	});

+	describe("#setter/getter", function() {


I did't understand this test describe block. because it doesn't have it block

Oh, I'll remove that.

jongmoon · 2018-01-26T02:14:12Z

test/unit/PanoImageRenderer/WebGLUtils.spec.js

@@ -43,8 +43,7 @@ describe("WebglUtils", function() {

        [
            {os: {name: "android", version: "4.3"}, browser: {name: "chrome"}},
-            {os: {name: "android", version: "4.4"}, browser: {name: "samsung internet"}},
-            {os: {name: "ios", version: "7"}, browser: {name: "safari"}}


Why remove {os: {name: "ios", version: "7"}, browser: {name: "safari"}}?

iOS 7 safari don't support webgl. So I removed conditional logic in isStableWebGL method. and added checking webgl support using isWebGLAvailable method before conditional logic in isStableWebGL.

In real iOS7 device, isStableWebGL will return false because it is not support webgl.
It's not valid test anymore before mocking no-webgl environment.

jongmoon · 2018-01-27T05:03:29Z

src/PanoImageRenderer/renderer/CubeRenderer.js

-		if (!image) {
-			return;
-		}
+	static updateTexture(gl, texture, image, imageConfig) {


texture param is not used.

jongmoon · 2018-01-27T05:08:00Z

src/PanoImageRenderer/renderer/SphereRenderer.js

@@ -50,6 +50,11 @@ export default class SphereRenderer extends Renderer {
 			}`;
 	}

+	static updateTexture(gl, texture, image) {
+		// Draw first frame
+		this.texImage2D(gl, image);


I think it's better to remove texImage2D method and replace this function with following

static updateTexture(gl, image) { gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE, image); }

jongmoon · 2018-01-27T05:15:50Z

test/manual/PanoViewer/Visible.html

@@ -155,8 +155,11 @@ <h2>Visible Activation</h2>
 			key = (new Date()).getTime() + idx;

 			instance = new eg.view360.PanoViewer(viewerEl,{
-					image: viewerEl.getAttribute("data-source"),
-					projectionType: "vertical_cubestrip"
+					image: {


@happyhj image should not have cubemap config

jongmoon · 2018-01-27T05:17:43Z

src/PanoImageRenderer/PanoImageRenderer.js

+	_updateTexture() {
+		this._renderer.updateTexture(
+			this.context,
+			this.texture,


@happyhj texture param is no more need

jongmoon

LGTM

…into CubeRenderer#123

happyhj added 5 commits January 22, 2018 18:04

skip: increase test theshold

e445be6

skip: increase threshhold

1a1356a

skip: removed support for legacy browsers

d13aa93

skip: add tests, missing math mehtods

a1f0f5e

jongmoon suggested changes Jan 23, 2018

View reviewed changes

happyhj added 5 commits January 25, 2018 17:24

skip: apply review

77fae4f

skip: apply review

84977bd

skip: apply review

ba65e8a

feat(PanoViewer): removed checksupport, add blacklisting feature

560e164

skip: update test

8f4258d

naver deleted a comment from coveralls Jan 25, 2018

jongmoon reviewed Jan 26, 2018

View reviewed changes

skip: apply review

b49c6bd

jongmoon reviewed Jan 27, 2018

View reviewed changes

happyhj added 4 commits January 29, 2018 11:26

skip: apply review

bd1080e

skip: apply review

5b5e707

chore(release): Release 2.0.2

99ab327

chore(release): merge 2.0.2-rc

1d5e243

jongmoon approved these changes Jan 30, 2018

View reviewed changes

happyhj added 2 commits January 30, 2018 12:41

skip: erge branch '3.0.0-rc' of https://github.com/naver/egjs-view360 …

bfc6e2d

…into CubeRenderer#123

skip: Merge branch 'master' of https://github.com/naver/egjs-view360 …

72d4283

…into CubeRenderer#123

happyhj merged commit 29a81bd into 3.0.0-rc Jan 30, 2018

jongmoon mentioned this pull request Mar 19, 2018

Update README for 3.0.0 #174

Closed

jongmoon deleted the CubeRenderer#123 branch March 29, 2018 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(PanoViewer): improve cubemap support #131

feat(PanoViewer): improve cubemap support #131

happyhj commented Jan 22, 2018 •

edited

Loading

jongmoon Jan 23, 2018

happyhj Jan 25, 2018

jongmoon Jan 23, 2018

happyhj Jan 25, 2018 •

edited

Loading

jongmoon Jan 23, 2018

happyhj Jan 25, 2018

jongmoon Jan 23, 2018

happyhj Jan 25, 2018

jongmoon Jan 23, 2018

happyhj Jan 25, 2018

jongmoon Jan 23, 2018

happyhj Jan 25, 2018

jongmoon Jan 26, 2018

happyhj Jan 26, 2018

jongmoon Jan 26, 2018

happyhj Jan 26, 2018

jongmoon Jan 26, 2018

happyhj Jan 26, 2018

jongmoon Jan 26, 2018

happyhj Jan 26, 2018

jongmoon Jan 27, 2018

jongmoon Jan 27, 2018

jongmoon Jan 27, 2018

jongmoon Jan 27, 2018

jongmoon left a comment

feat(PanoViewer): improve cubemap support #131

feat(PanoViewer): improve cubemap support #131

Conversation

happyhj commented Jan 22, 2018 • edited Loading

Issue

Details

feat

fix

test

chore

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

happyhj Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongmoon left a comment

Choose a reason for hiding this comment

happyhj commented Jan 22, 2018 •

edited

Loading

happyhj Jan 25, 2018 •

edited

Loading