Refactor preview.js and model_stats.js#86
Conversation
|
@AyushDharDubey, I have solved the issue. Do, tell me if any further changes are needed. |
There was a problem hiding this comment.
Thanks a pile, this is exactly what was required, to load the gltf object once and then reuse the same object: a single-pass architecture.
I have left some minor comments, Additionally,
on setUpRenderPane call it would be nice to use named-parameters:
setUpRenderPane({
onLoaded: function (gltf) {
const stats = new ModelStats();
stats.processModel(gltf);
}
});for better readability.
| window.setUpRenderPane = setUpRenderPane; | ||
| window.displayPreview = displayPreview; |
There was a problem hiding this comment.
These two no longer exist independently in the scope and need to be removed.
There was a problem hiding this comment.
Sure, removed from [preview.js] as requested, and exposed from [main.js] for backward compatibility while templates are still using global calls.
…ne and displayPreview Removal globalization
Okay, Changed it to use named paramaters |
|
Implemented all the fixes as suggested by you, plss review again and update me. |
AyushDharDubey
left a comment
AyushDharDubey
left a comment
There was a problem hiding this comment.
Apart from this, the commit history would need a rebase to display meaningful commit messages pertaining to the actual changes.
| export function setUpRenderPane({ onLoaded } = {}) { | ||
| const elems = document.querySelectorAll('div.render-pane'); | ||
|
|
||
| fullscreenButton.addEventListener('click', function (event) { | ||
| const renderPane = document.querySelector('.render-pane'); | ||
| for (const elem of elems) { | ||
| const model_id = elem.dataset.model; | ||
| const revision = elem.dataset.revision; | ||
| const url = "/api/model/" + model_id + "/" + revision; | ||
|
|
||
| if (!document.fullscreenElement && renderPane.requestFullscreen) { | ||
| renderPane.requestFullscreen(); | ||
| } else if (document.exitFullscreen) { | ||
| document.exitFullscreen(); | ||
| } | ||
| }); | ||
| const preview = new ModelPreview(elem.id); | ||
| preview.loadAndDisplay(url, onLoaded); | ||
| } | ||
| } | ||
|
|
||
| document.addEventListener('fullscreenchange', function (event) { | ||
| handleFullscreenChange(threeInstance); | ||
| }); | ||
| export function displayPreview(elementId, url, options = {}, onLoaded) { | ||
| const preview = new ModelPreview(elementId, options); | ||
| preview.loadAndDisplay(url, onLoaded); |
There was a problem hiding this comment.
It's better to move these logic in the respective templates wherever they are applicable, the static assets would better contain only the reusable objects, rather than helper functions specific to page
|
I think now also, there are some indentations and space flows. Lemme check them also |
|
Okay, Now everything is fixed and corrected. You can have a review and then look forward for any other changes. |
|
Any updates? |
|
It’s difficult to identify the actual changes due to the large number of redundant indentation and formatting updates, which don’t affect the underlying logic. There are still excessive reformatting changes in A full reformat of the rendering logic is a substantial task in itself. It would be preferable to reopen a cleaner PR containing only the intended changes... |
|
Ok, I'll do that |
|
closing this due to inactivity. |
3 participants
In this PR, I have modified the
Functionstructure code to theClasstype in model_stats.js and preview.js which was actually required for many crucial reasons such as the ownership of the variables, lifecycle of the components, clean code and expecially no memory leaks.Another change is that, We have removed the separate GLB File Loading for both the components stats and preview one.
This was achieved by loading the GLB file in the preview.js only and then parsing it and passing as a reference to the model_stats component, which then uses it to calculate the stats such as faceCount, meshCount etc.