Skip to content
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

Removed Geometry generators #21018

Merged
merged 7 commits into from
Jan 8, 2021
Merged

Removed Geometry generators #21018

merged 7 commits into from
Jan 8, 2021

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Jan 6, 2021

Trying to remove THREE.Geometry.

@mrdoob mrdoob added this to the r125 milestone Jan 6, 2021
@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 6, 2021

Assuming Geometry is later moved to the examples and renamed to LegacyGeometry. And BufferGeometry is renamed to Geometry. Can we then rename the generators back to e.g. BoxGeometry?

By adding an alias for each geometry generator, names like BoxBufferGeometry should still work.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 6, 2021

I'll give that a try tomorrow 👍

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 6, 2021

BTW: webgl_buffergeometry_constructed_from_geometry is failing in the E2E test since it's the last example that uses ExtrudeGeometry.

TBH, it makes no sense to convert it to BufferGeometry since its purpose is to show the construction of a BufferGeometry from Geometry. I think we can delete it 👍 .

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 7, 2021

Since #21021 is now in dev, I hope rebasing should fix unit and E2E tests.

@mrdoob mrdoob changed the title Removed Geometry primitives. Remove Geometry from src/ Jan 7, 2021
@mrdoob
Copy link
Owner Author

mrdoob commented Jan 7, 2021

Assuming Geometry is later moved to the examples and renamed to LegacyGeometry. And BufferGeometry is renamed to Geometry. Can we then rename the generators back to e.g. BoxGeometry?

Thinking more about this... I'm not sure we can do that.

If we rename BufferGeometry to Geometry then we'll also have to change all the .isBufferGeometry checks everywhere.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 7, 2021

Couldn't we just add the isBufferGeometry property to Geometry (for backwards compatibility)?

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 7, 2021

I think that complicates things more...

I would rather keep BufferGeometry as the base and introduce something like MeshGeometry, LineGeometry, PointsGeometry and make them extend BufferGeometry.

That way we'll end up with BoxMeshGeometry, CircleMeshGeometry, etc

I'd also allow us to check geometry.isMeshGeometry in Line and warn the user that things are not likely to look right.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 7, 2021

Okay, that's fine with me. I remember we discussed the introduction of specific geometry classes earlier.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 7, 2021

Yeah... I'm still unsure about the approach, but feels way less scary than renaming BufferGeometry to Geometry 😅

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 7, 2021

I'm just a bit sad that the name Geometry is now somewhat reserved and not usable for the actual primal geometry class (which is BufferGeometry).

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 7, 2021

Yeah, I understand... 🤗

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 7, 2021

Maybe we can reconsider a rename later. When LegacyGeometry was part of the examples for a year or so.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 8, 2021

Possible next steps:

  • Move the class DirectGeometry into Geometry.js.
  • Remove the following methods in BufferGeometry: fromGeometry(), fromDirectGeometry(), setFromObject() and updateFromObject(). Enhance THREE.Legacy.js and log console errors for the above methods.
  • Introduce Geometry.toBufferGeometry().
  • Stop rendering Geometry. This requires a small change in WebGLGeometries and WebGLObjects.
  • Move Geometry.js to examples/js(m)/geometries.
  • Remove all isGeometry checks in the code base. Instead, check for geometry.isBufferGeometry !== true.

@mrdoob mrdoob changed the title Remove Geometry from src/ Removed Geometry generators Jan 8, 2021
@mrdoob
Copy link
Owner Author

mrdoob commented Jan 8, 2021

I'll merge this for now and open another PR with these steps.

@mrdoob mrdoob marked this pull request as ready for review January 8, 2021 18:20
@mrdoob mrdoob merged commit 247bde9 into dev Jan 8, 2021
@mrdoob mrdoob deleted the geometry branch January 8, 2021 18:32
@mrdoob
Copy link
Owner Author

mrdoob commented Jan 8, 2021

Follow-up: #21031

This was referenced Jan 8, 2021
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.

None yet

2 participants