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.

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

Removing nft version in the build libs #407

Merged
merged 20 commits into from
Mar 20, 2022
Merged

Removing nft version in the build libs #407

merged 20 commits into from
Mar 20, 2022

Conversation

kalwalt
Copy link
Member

@kalwalt kalwalt commented Mar 16, 2022

⚠️ 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?

This PR will remove the double build libs (with or without nft extension). Because an issue with aframe image tracking this will be done only for threex and ar.js namespace.
Can it be referenced to an Issue? If so what is the issue # ?
see #402
How can we test it?

This still a WIP.
Summary

Does this PR introduce a breaking change?
For threex code if you want to use nft markers, you will import the ar-threex.js build lib instead of ar-threex-nft.js. That's all.
Please TEST your PR before proposing it. Specify here what device you have used for tests, version of OS and version of Browser
Tested on desktop Ubuntu 20.04.
Other information
This PR also add prettier to format the code and a simple github action script to test linting. Husy with lint-staged to format the code when a new commit is created to enforce code styling.

@kalwalt kalwalt marked this pull request as draft March 16, 2022 15:56
@kalwalt
Copy link
Member Author

kalwalt commented Mar 16, 2022

As said in issue #402 the aframe nft example are not working correctly. I'm trying to catch the bug. i tested this commit 841be3d and the examples were working. I will continue this cehcking until i will found the bug.

@kalwalt
Copy link
Member Author

kalwalt commented Mar 17, 2022

version AR.js 3.3.3-es6-beta-03 seems fine i tested also the commit 604d879 and it's fine.

@kalwalt
Copy link
Member Author

kalwalt commented Mar 17, 2022

worker can't load and continue loading resources with this commit 814435d So someting wrong when adding the event listener or initializig the arContext.

@kalwalt
Copy link
Member Author

kalwalt commented Mar 17, 2022

if i remove this code from arjs-session.js:

window.addEventListener('arjs-video-loaded', function () {
		arContext.init(() => {

                arContext.arController.orientation = getSourceOrientation();
	        arContext.arController.options.orientation = getSourceOrientation();

		})
	})

	function getSourceOrientation() {
		console.log(_this);
				if (!_this) {
						return null;
				}

				console.log(
						'actual source dimensions',
						arSource.domElement.clientWidth,
						arSource.domElement.clientHeight
				);

				if (arSource.domElement.clientWidth > arSource.domElement.clientHeight) {
						console.log('source orientation', 'landscape');
						return 'landscape';
				} else {
						console.log('source orientation', 'portrait');
						return 'portrait';
				}
		}

The worker start to load all the resources, camera_para.dat and the NFT markers as well. I think i will create two arjs-session.js one with this routine for non nft and another without for nft. So until i don't find another solution we will have in the build aframe-ar.js and aframe-ar-nft.js.

- new arjs-session-nft.js
- double build libs for aframe
@kalwalt kalwalt marked this pull request as ready for review March 17, 2022 15:29
@kalwalt
Copy link
Member Author

kalwalt commented Mar 17, 2022

github actions fails, i think an issue with npm? or i missed something?

@kalwalt
Copy link
Member Author

kalwalt commented Mar 17, 2022

from twitter: https://twitter.com/githubstatus/status/1504526048591073293?s=20 seems some problems with github action

@kalwalt
Copy link
Member Author

kalwalt commented Mar 17, 2022

Now the action script run successfully the test script!

@kalwalt kalwalt self-assigned this Mar 17, 2022
@kalwalt kalwalt added bug Something isn't working enhancement New feature or request github action labels Mar 17, 2022
@kalwalt kalwalt force-pushed the removing-nft-branching branch from a66a967 to 4c73595 Compare March 19, 2022 16:11
@kalwalt
Copy link
Member Author

kalwalt commented Mar 20, 2022

The README file need to be updated about which lib files to use for the different namespaces and nft or if location based or not. Will do this as well.

@kalwalt
Copy link
Member Author

kalwalt commented Mar 20, 2022

@nickw1 i will merge this as it will fix some important bugs and add prettier to format the code. When it will be merged we need to test if formatting works when every commit is created. We need also to format code after this PR. Prettier will format the code only .js files for now. We can include also other types (.css html md) but we can do in another step. Probably we can change the prettier configuration, to format the code in a better way, if you have some suggestions let me know.

@nickw1
Copy link
Collaborator

nickw1 commented Mar 20, 2022

@kalwalt ok looks good. No suggestions just yet but will see what results the code formatting produces.

@kalwalt kalwalt merged commit 5d439aa into dev Mar 20, 2022
@kalwalt
Copy link
Member Author

kalwalt commented Mar 20, 2022

I think we need to update also the AR-js-docs because are not updated with the latest changes.

@kalwalt kalwalt deleted the removing-nft-branching branch December 22, 2024 22:03
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 github action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants