Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 54 additions & 5 deletions packages/engine/Source/Core/GeometryFactory.js
Original file line number Diff line number Diff line change
@@ -1,21 +1,70 @@
import DeveloperError from "../Core/DeveloperError.js";

/**
* Base class for all geometry creation utility classes that can be passed to {@link GeometryInstance}
* for asynchronous geometry creation.
* Describes a geometry type that can be converted into a {@link Geometry}.
* <p>
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to add the <p> tags yourself. This should be handled by JSDoc itself AFAIK.

* Implementations of this interface are the core geometry "description" classes
* such as {@link BoxGeometry}, {@link RectangleGeometry},
* {@link EllipsoidGeometry}, and other classes with a static
* <code>createGeometry</code> function. Instances of these classes can be
* passed to {@link GeometryInstance} and {@link Primitive} to have their
* vertices and indices created either asynchronously on a web worker or
* synchronously on the main thread.
* </p>
* <p>
* This type describes an interface and is not intended to be instantiated
* directly.
* </p>
*
* @alias GeometryFactory
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this needs an alias since it matches the function name itself

* @constructor
* @class
* @abstract
*
* @see BoxGeometry
* @see BoxOutlineGeometry
* @see CircleGeometry
* @see CircleOutlineGeometry
* @see CoplanarPolygonGeometry
* @see CoplanarPolygonOutlineGeometry
* @see CorridorGeometry
* @see CorridorOutlineGeometry
* @see CylinderGeometry
* @see CylinderOutlineGeometry
* @see EllipseGeometry
* @see EllipseOutlineGeometry
* @see EllipsoidGeometry
* @see EllipsoidOutlineGeometry
* @see FrustumGeometry
* @see FrustumOutlineGeometry
* @see GroundPolylineGeometry
* @see PlaneGeometry
* @see PlaneOutlineGeometry
* @see PolygonGeometry
* @see PolygonOutlineGeometry
* @see PolylineGeometry
* @see PolylineVolumeGeometry
* @see PolylineVolumeOutlineGeometry
* @see RectangleGeometry
* @see RectangleOutlineGeometry
* @see SimplePolylineGeometry
* @see SphereGeometry
* @see SphereOutlineGeometry
* @see WallGeometry
* @see WallOutlineGeometry
Comment on lines +50 to +53
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm kinda on the fence about the usefullness of this simply because it's such a big list. I could see it easily going out of sync with the actual classes. I don't think there's been much change in this area so maybe it doesn't matter.

@donmccurdy or @javagl this is sorta related to recent discussions we've had around classes and inheritance. Any thoughts here?

Copy link
Member

Choose a reason for hiding this comment

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

Thank you @MohammadShujaullah for compiling the list! However I think I agree with @jjspace, I'd (personally) be hesitant to commit to maintaining such a list by hand.

I can't speak for every code editor, but using TypeScript language services in Zed, there are "Find All References" and "Go to Implementation" tools built into the editor. These don't work on GeometryFactory for me today (TS language service limitations with JSDoc probably), unless I either:

Getting that information out of the language server, and into published docs, might require more steps — if that's the goal. But at least we'd have confidence that the list will remain up to date.

If we do merge #13104, adding the missing @extends X or @implements X annotations could be a good next step?

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion on this particular case.

Pro:

Con:

  • Maintenance (but that's a weak point, given the 'Stability' above)
  • It may become obsolete with better tooling around TS/JSDoc (but that may take months or years...)

Some of the back-and-forth links are currently broken anyhow (e.g. InterpolationAlgorithm or Packable - see #11749 (comment) ). As long as we don't have a clean(er) concept of visibility and automatisms for generating such links, I think that adding the list here is OK, and be it only to know into which classes we'll have to insert @extends GeometryFactory after the corresponding tooling was set up 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the input @donmccurdy and @javagl. I think given the stability of this area which hasn't changed much I'm going to lean towards "any info is better than no info" for now. These can stay.

We should definitely look into improving this area in the future since things like this should be automated. This may come "free" or easier after some of the class and/or TS refactoring @donmccurdy mentioned but that's far outside the scope for this PR.

*/
function GeometryFactory() {
DeveloperError.throwInstantiationError();
}

/**
* Returns a geometry.
* Creates a {@link Geometry} from a geometry description.
* <p>
* Concrete geometry classes (for example {@link RectangleGeometry}) implement
* this function as a static method that takes an instance of the corresponding
* geometry description and returns the computed vertices and indices.
* </p>
*
* @param {GeometryFactory} geometryFactory A description of the circle.
* @param {GeometryFactory} geometryFactory The geometry description to create.
* @returns {Geometry|undefined} The computed vertices and indices.
*/
GeometryFactory.createGeometry = function (geometryFactory) {
Expand Down
6 changes: 6 additions & 0 deletions packages/engine/Source/Core/GeometryInstance.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ import Matrix4 from "./Matrix4.js";
*
* @param {object} options Object with the following properties:
* @param {Geometry|GeometryFactory} options.geometry The geometry to instance.
* When a {@link Geometry} is passed, it will be used directly. When an
* object implementing {@link GeometryFactory} is passed (for example an
* instance of {@link RectangleGeometry} or {@link EllipsoidGeometry}),
* the actual geometry will be created either asynchronously on a web
* worker or synchronously on the main thread depending on the
* {@link Primitive} options.
* @param {Matrix4} [options.modelMatrix=Matrix4.IDENTITY] The model matrix that transforms to transform the geometry from model to world coordinates.
* @param {object} [options.id] A user-defined object to return when the instance is picked with {@link Scene#pick} or get/set per-instance attributes with {@link Primitive#getGeometryInstanceAttributes}.
* @param {object} [options.attributes] Per-instance attributes like a show or color attribute shown in the example below.
Expand Down
Loading