Skip to content

Cleanup new Color/Matrix() #26024

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

Cleanup new Color/Matrix() #26024

wants to merge 9 commits into from

Conversation

@LeviPesin
Copy link
Contributor

@LeviPesin LeviPesin commented May 9, 2023 •
edited
Loading

Related issue: #26021

Description

Some clean up.

@github-actions
Copy link

github-actions bot commented May 9, 2023 •
edited
Loading

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
642.3 kB (159 kB) 642.3 kB (159 kB) -27 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
432.2 kB (104.7 kB) 432.2 kB (104.7 kB) +0 B

@LeviPesin
Copy link
Contributor Author

LeviPesin commented May 9, 2023

Seems like this breaks something... I will test it locally.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 9, 2023 •
edited
Loading

Please consider to update not too many things at once.

@LeviPesin
Copy link
Contributor Author

LeviPesin commented May 9, 2023

OK, reverted ShaderNode changes.

@LeviPesin LeviPesin mentioned this pull request May 9, 2023
Merged
mrdoob
mrdoob reviewed May 10, 2023
View reviewed changes
@mrdoob mrdoob added this to the r153 milestone May 10, 2023
mrdoob
mrdoob reviewed May 12, 2023
View reviewed changes
examples/jsm/loaders/NRRDLoader.js
volume.spacing = [ spacingX, spacingY, spacingZ ];


// Create IJKtoRAS matrix
Copy link
Owner

@mrdoob mrdoob May 12, 2023

Choose a reason for hiding this comment

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

Shouldn't this comment be moved too?

Copy link
Contributor Author

@LeviPesin LeviPesin May 13, 2023

Choose a reason for hiding this comment

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

I think no, because transitionMatrix is a part of building IJKtoRAS matrix.

@Mugen87 Mugen87 modified the milestones: r153, r154 Jun 1, 2023
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 1, 2023 •
edited
Loading

There are too many uncertainties in this PR and I don't think the new code styles are better then the previous ones. In no way this type of "clean-up" PR should require such an in-depth review. I would say it's better to close the PR.

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Jun 1, 2023

There was only one "uncertainty" in this PR (assigning the same object to two fields). I don't see why it can't be merged?

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Jun 11, 2023

Can we merge this PR?

@Mugen87 Mugen87 mentioned this pull request Jun 22, 2023
Closed
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 27, 2023

Same as #26317 (comment).

@Mugen87 Mugen87 closed this Jun 27, 2023
@Mugen87 Mugen87 modified the milestones: r154, r153 Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrdoob mrdoob mrdoob left review comments

@Mugen87 Mugen87 Mugen87 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LeviPesin @Mugen87 @mrdoob