Skip to content

Conversation

@WestLangley
Copy link
Collaborator

Inspired by #6457 (comment).

It is _gl.LINEAR_MIPMAP_LINEAR, not _gl.LINEAR_MIP_MAP_LINEAR.

The boolean generateMipmaps uses the correct nomenclature.

This PR is a first-step. We can discuss the best way to proceed.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 7, 2019

I think I would already use the new constants in the code base. Just introducing them in constants is a very small step.

@WestLangley
Copy link
Collaborator Author

This is ridiculous. The PR is too small?

Let's merge PRs and make progress.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 8, 2019

PRs should represent meaningful bundles of work. I don't think it makes sense to just add a few constants to the lib. If you want to make progress, you should already introduce them in the rest of the code base.

BTW: #16671 has a similar problem. You have introduced .fromEquirectangularTexture() but the method is still not used once in the entire code base (and undocumented). It would have been better to directly replace all occurrences of EquirectangularToCubeGenerator and CubemapGenerator.

@WestLangley
Copy link
Collaborator Author

This PR adds support for the new nomenclature ("Mipmap"), while maintaining backwards-compatibility with the old ("MipMap").

I'll file another PR to use the new nomenclature in the source code, while leaving the examples as-is.

If that works, I'll update the examples to use the new nomenclature as a final step.

I think we want to maintain backwards-compatibility, so that means supporting both the new and the old nomenclatures.

Maybe someone has a suggestion for how the old nomenclature can be deprecated...

@WestLangley WestLangley merged commit 0899532 into mrdoob:dev Jul 9, 2019
@WestLangley WestLangley mentioned this pull request Jul 9, 2019
@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 9, 2019

Again, I'm against this type of PR style. Remembers me at salami tactics....

@WestLangley WestLangley deleted the dev_mipmap_constants branch July 11, 2019 01:18
@mrdoob
Copy link
Owner

mrdoob commented Jul 31, 2019

TIL about salami tactics 😁

Doing perfect PRs can be a bit taxing, so I'm okay with solving issues in multiple PRs.

@mrdoob
Copy link
Owner

mrdoob commented Jul 31, 2019

Maybe someone has a suggestion for how the old nomenclature can be deprecated...

Maybe we should turn them into getters and log a warning when accessed?

@WestLangley
Copy link
Collaborator Author

Maybe we should turn them into getters

Hmm... how is that done? 🤔

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