Skip to content

Conversation

@kalwalt
Copy link
Member

@kalwalt kalwalt commented Mar 17, 2020

⚠️ All PRs have to be done versus 'dev' branch, so be aware of that, or we'll close your issue ⚠️

What kind of change does this PR introduce?

Fix for issue #59 missed rotation on X axis.
Can it be referenced to an Issue? If so what is the issue # ?
issue #59
How can we test it?

Test the basic and dev exemples (three.js)
Summary

The object 3D is not rotated correctly, we should apply a rotation on the X axis to fix the problem.
Does this PR introduce a breaking change?

Please TEST your PR before proposing it. Specify here what device you have used for tests, version of OS and version of Browser

Other information
A said in the issue #59 this bug was introduced while deleting the tango and aruco code, we (me) deleted a relevant part of the code

https://github.com/jeromeetienne/AR.js/blob/024318c67121bd57045186b83b42f10c6560a34a/three.js/src/threex/threex-armarkercontrols.js#L130-L135

Applying again the rotation X will solve the issue.

List of features / to do list:

  • fix for issue bug with rotation  #59 reapplying the rotation matrix X to threex-armarkercontrols.js
  • upgrading artoolkit version for ar.js and aframe-ar.js ( previuosly only for ar-nft.js, aframe-nft-ar.js) This will be done in another PR.
  • removing the multiplication for the modelviewMatrix with ProjectionAxisTransformMatrix see line 258 in threex-artoolkitcontext.js
  • fix examples (three.js and aframe) for the position of the 3d objects (y instead of z)
  • removing old artoolkit.min.js libs This will be done in another PR.
  • removing references/links to the old Ar.js repository
  • testing the examples on Desktop and Mobile device.

JamesBotterill and others added 2 commits March 16, 2020 15:30
- upgrading artoolkit.min.js to the new version
- upgrading some examples
@kalwalt kalwalt added bug Something isn't working enhancement New feature or request labels Mar 17, 2020
@kalwalt kalwalt self-assigned this Mar 17, 2020
@kalwalt kalwalt force-pushed the feature-artoolkit-new branch from f91ad35 to a94f8cc Compare March 17, 2020 13:14
@kalwalt
Copy link
Member Author

kalwalt commented Mar 17, 2020

Empirical demonstration with dev.html example, threex.artoolkitcontext.js without
projectionMatrix.multiply(this._artoolkitProjectionAxisTransformMatrix)
and artoolkit-nft.min.js

new-artoolkitcontex

dev.html example threex.artoolkitcontext.js with
projectionMatrix.multiply(this._artoolkitProjectionAxisTransformMatrix)
and artoolkit.min.js

old-artoolkitcontext

Note the three axes helper is oriented in the same way in both the images.

@kalwalt
Copy link
Member Author

kalwalt commented Mar 17, 2020

I did also this gist for proving the theory https://gist.github.com/kalwalt/528e722706847ea523c489947d07bc8c

@kalwalt
Copy link
Member Author

kalwalt commented Mar 18, 2020

arjs-session.html example without multiply X matrix:

wrong-arjs-session

with the right multiplication matrix, working as should:

right-arjs-session
the white grid must be in the same plan of the marker.
In the next hours will check also the nft examples (threejs and aframe).

- fix examples y instead of z positioning Threejs and aframe)

- fix position nft examples
@kalwalt
Copy link
Member Author

kalwalt commented Mar 18, 2020

I added the rotation X matrix also to the arMarkerControls-nft, because you could use in combination with a Pattern Marker. Soon i will provide a codepen example.

@kalwalt
Copy link
Member Author

kalwalt commented Mar 18, 2020

It's possible but i don't reccomend to work with both, test this codepen: https://codepen.io/kalwalt/pen/LYVrQKd test with an Hiro marker and the trex image. When pointing to the hiro marker you will see two blue cube flashing on the lefte and after on the right. I think we should find a solution also for this, not of course in this PR.

@kalwalt kalwalt linked an issue Mar 18, 2020 that may be closed by this pull request
@kalwalt
Copy link
Member Author

kalwalt commented Mar 18, 2020

Testing the threejs examples with Mobile, the cube is distorted in portrait mode:
InShot_20200318_175250952 instead in landscape mode works as expected.
This happens for almost the examples. default-thinner-border.html and test-runner-html not because use an image as source.

@kalwalt
Copy link
Member Author

kalwalt commented Mar 18, 2020

We get different Matrix from Camera because this changes in Artoolkit5 arglCameraFrustum was removed in favour to arglCameraFrustumRH in this commit see how i changed the code in ARToolkitJS.cpp
artoolkitx/jsartoolkit5@b3b9682#diff-e307757acae7e50b4037668bc9d31b32

@kalwalt
Copy link
Member Author

kalwalt commented Mar 19, 2020

I'm testing if adding back the old arglCameraFrustum method has some effect on the code
my testing branch https://github.com/kalwalt/jsartoolkit5/tree/fix-arglCameraFrustum .
This not change anything on Mobile Device. You get the same result, and most important. you will have the same result if you use the old artoolkit.min.js without artoolkit.api.js. So artoolkit.api.js is necessary because add some extra code (and probably was modified for this reason?) to solve the ratio issue.

@kalwalt
Copy link
Member Author

kalwalt commented Mar 19, 2020

For sure is relative to changes in artoolkit.api.js see this commit in the old AR.js repo, there are changes in _copyImageToHeap to the portrait mode. Maybe we should make this changes in Ar.js?

Add video stop on marker being lost
@kalwalt
Copy link
Member Author

kalwalt commented Mar 20, 2020

I found that if you print the orientation from arController give wrong result:

arToolkitContext.init(function onCompleted(){
		console.log(arToolkitContext.arController.orientation);
// other code here
}

in portrait mode should give portrait instead give landscape
This not happens in jsartoolkit5 it gives me the right result. I think i will open another issue for this.
Because it works well under jsartoolkit5 we have found the reason of this.

@kalwalt kalwalt changed the title [WIP] upgrading artoolkit.min.js and fix for rotation bug [WIP] fix for rotation X bug Mar 20, 2020
@kalwalt
Copy link
Member Author

kalwalt commented Mar 20, 2020

I think that is better to merge the fix for the bug in the Rotation on the X axis, and after make the upgrade to the new artoolkit lib. This PR need testing anyone want to test is welcome! 🙂

This was referenced Apr 6, 2020
nicolocarpignoli and others added 2 commits April 9, 2020 12:24
Addressing issue AR-js-org#70: Version of gps-camera and gps-entity-place using Spherical Mercator p…
@kalwalt
Copy link
Member Author

kalwalt commented Apr 11, 2020

Hi @fcor have you tested this PR?

@kalwalt
Copy link
Member Author

kalwalt commented Apr 11, 2020

For anyone wants to test this PR try this codepen: https://codepen.io/kalwalt/pen/ZEbYwdq you should see a red cone onto an Hiro marker: the corner pointing outside the marker and placed correctly on it.

@kalwalt kalwalt merged commit 93cbc7f into AR-js-org:dev Apr 15, 2020
@kalwalt kalwalt mentioned this pull request Apr 19, 2020
@nicolocarpignoli
Copy link
Member

@kalwalt please write on CHANGELOG file, directly on dev branch, this modify :) thanks!

next version will be 3.1 so write under 3.1

@kalwalt
Copy link
Member Author

kalwalt commented Apr 22, 2020

@kalwalt please write on CHANGELOG file, directly on dev branch, this modify :) thanks!

next version will be 3.1 so write under 3.1

@nicolocarpignoli i will update the CHANGELOG 🙂

kalwalt added a commit that referenced this pull request Mar 30, 2021
@kalwalt kalwalt mentioned this pull request May 10, 2021
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug with rotation

4 participants