Skip to content

[ENG-8627] | Fix .3ds files not rendering in MFR viewer #404

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

Open
wants to merge 5 commits into
base: feature/buff-worms
Choose a base branch
from

Conversation

sh-andriy
Copy link

Ticket

Purpose

Changes

Side effects

QA Notes

Deployment Notes

Copy link
Collaborator

@opaduchak opaduchak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a few minor remarks regarding codestyle

Comment on lines 583 to 585
var materialFaces = this._cur_obj.materialFaces || {};
var sawMaterial = false;
for (var materialName in materialFaces) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It's better to use let instead of var


var currentMaterial = this._materials[materialName];
if (!currentMaterial) {
if (JSC3D.console) JSC3D.console.logWarning('3DS: missing material "' + materialName + '", using defaults.');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't use oneline if statements

Suggested change
if (JSC3D.console) JSC3D.console.logWarning('3DS: missing material "' + materialName + '", using defaults.');
if (JSC3D.console) {
JSC3D.console.logWarning('3DS: missing material "' + materialName + '", using defaults.');
}

@sh-andriy sh-andriy requested a review from opaduchak August 20, 2025 16:18
@coveralls
Copy link

Coverage Status

coverage: 68.714%. remained the same
when pulling a507620 on sh-andriy:fix/ENG-8627
into a080e58 on CenterForOpenScience:feature/buff-worms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants