-
Notifications
You must be signed in to change notification settings - Fork 15
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
Visualize 3D models locally without relying on sketchfab iframe #2091
base: develop
Are you sure you want to change the base?
Visualize 3D models locally without relying on sketchfab iframe #2091
Conversation
Hi @luistoptal, prasie: Was able to associate this https://sketchfab.com/3d-models/beet-03-c63d100fb76042ecab2ec87f73da323e stl file to dataset 100006, and display it on Chrome browser. question: The 3D image cannot be shown in Safari nor Firefox browsers, can you help further look into it? SafariFirefox |
@kencho51 looking at the images, it looks as if the CSS is not loaded. Because not only the 3D model is not shown but the buttons look as if the custom styles applied to them were not working. Can you try:
|
Hi @luistoptal,
After hard refresh/empty the cache in safari and firefox, the 3D image can be displayed now. |
Hi @luistoptal, question: Would it be better to have acceptance tests/accessibility tests as there is an extra
@rija, @pli888 , what do you think? praise: I was able to deploy this PR to my staging, and display the 3D images from the above files stored in the s3 bucket. |
@kencho51 that would work since the viewer simply loads and displays files accessible via public links, thanks for adding these files, now they can be used to update the tests there is one test for the 3D models tab already present, it hadn't been updated |
I extended the test that checks for the 3D Model tab |
Hi @luistoptal, issue: I have further tested this PR in my production, and the 3D image actually cannot be displayed properly as below: and figured out that the following external scripts were blocked by the CSP:
suggestion: The fix could be made by adding |
Hi @luistoptal, issue: The
|
@kencho51 I think it would be simpler if one dataset is created already with a 3D model for acceptance testing (one of the S3 links you posted above), what would be the right place to add this? (I see many sql files that are probably used for testing, but not sure which ones are ok and which ones should be deprecated) |
Hi @luistoptal, suggestion: For the test, you could:
Then for the implementation, you could add the scenarios to
And then you execute the test as below:
issue: Got this error in production suggestion: The fix could be made by adding |
Hi @luistoptal, Please use the model files from these links:
|
@kencho51 I added your suggestions, that was very helpful for some reason I had to shut down docker compose and re run up.sh to register the db entires I also added the wasaby source to all |
features/dataset-view.feature
Outdated
@ok | ||
Scenario: 3D Viewer | ||
Scenario: 3D Viewer | ||
Given I am not logged in to Gigadb web site | ||
And I have added "3D Viewer" link "https://sketchfab.com/models/ea49d0dd500647cbb4b61ad5ca9e659a" to dataset "101001" | ||
And I have added "3D Viewer" link "https://s3.ap-northeast-1.wasabisys.com/test-gigadb-datasets/3d-models/100006/GeoB8502_865cm_Shell-4.obj" to dataset "101001" | ||
When I go to "/dataset/101001" | ||
Then I should see "3D Models" tab with text "3D Models:" | ||
When I click on the "3D Models" button | ||
Then I should see a "select.test-model-selector" element | ||
And I should see a "select.test-model-selector > option" element | ||
And I should see "GeoB8502_865cm_Shell-4.obj" in the "select.test-model-selector > option" element | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: This scenario can be removed, since it has been moved to tests/acceptance/DatasetView.feature
already.
sql/3D_Viewer_101001_test_data.sql
Outdated
insert into external_link_type(id, name) | ||
select 5, '3D Models' | ||
where not exists ( | ||
select 1 from external_link_type | ||
where id = 5 and name = '3D Models' | ||
); | ||
insert into external_link(dataset_id, url, external_link_type_id) values(80,'https://s3.ap-northeast-1.wasabisys.com/test-gigadb-datasets/3d-models/100006/GeoB8502_865cm_Shell-4.obj',5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: The changes at here should be reverted, as only data from data/dev
will be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am deleting this file as I think it serves no purpose anymore
tests/acceptance/DatasetView.feature
Outdated
Then I should see "3D Models" | ||
|
||
@wip @issue-2054 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Change the @wip
to @ok
if the scenario is passing, the wip means work in progress
.
@@ -413,10 +413,7 @@ public function iShouldNotSeeTabWithTable($arg1, TableNode $table) | |||
*/ | |||
public function iHaveAddedLinkToDataset($arg1, $arg2, $arg3) | |||
{ | |||
if ("3D Viewer" == $arg1 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kencho51 since I deleted the 3D Models test, I am also deleting this switch case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @luistoptal,
With all the requested changes fixed and the 3D models can be displayed in my production, so LGTM, happy to approve.
I noticed a bug I need to fix |
fixed |
Pull request for issue: #2054
This is a pull request for the following functionalities:
How to test?
The following steps describe how I test this widget locally (if you know of a better way, please be my guest and I would love to know 🙂):
You can test error states if you repeat the above process by adding a file that is not a valid 3d model or not a LAS, STL, PLY or OBJ file (or you can just add a
throw new Error('Ooops')
in the line beforeawait loadModel(file);
)Example of error message:
How have functionalities been implemented?
3D models were previously visualized within an iframe taking a sketchfab url as input. This PR gets rid of the sketchfab iframe and replaces it with a JS widget that makes use of the three.js library to load and display an interactive 3D model
The main requirement is to load STL and OBJ file formats, and also (less common) PLY and LAS
Different options to locally load the model were considered (refer to the original ticket for some). I decided to use three.js because:
Other options I checked had very low or null adoption and / or did not fully cover the use case. I opted for the widely adopted solution.
The changes can be grouped as follows:
protected/js/model-viewer
protected/views/shared/_model_viewer.php
renders the html for the widget, and loads the necessary scripts;head
as a import map. That way, the three.js library can be imported as a module in the widget scriptsYii::app()->assetManager->forceCopy = YII_DEBUG;
makes sure the assets are rebuilt and not cached in development (NOTE: I am unclear on whether this targets specifically local dev, is this the correct solution? If this is a blocker, this line can be removed, but I found it necessary to add this line during local dev)protected/views/dataset/view.php
. This page contains a switch statement that renders elements such an iframe depending on the external link types. In the switch statement, the links are handled one by one. In the case of 3D models, I deleted that case from the switch statement, and passed them to the partial asdata
property. Note that I had to do this because the partial, rather than handling individual links one by one, takes them all to build a select input from which to select the model to loadless/modules/model-viewer.less
takes care of all the styles related to the viewerThat covers all the changes outside of the
protected/js/model-viewer
folder, now related to the widget itselfmodelViewer
function and dig deeper only if curious about implementation detailsmodelViewer
connects the two modules and initializes themAny issues with implementation?
isProduction
value inprotected/js/model-viewer/helpers/logger.js
Any changes to automated tests?
All tests pass