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.

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

Mod bufferobject #2981

Merged
merged 8 commits into from
Feb 6, 2024
Merged

Mod bufferobject #2981

merged 8 commits into from
Feb 6, 2024

Conversation

hjunqq
Copy link
Contributor

@hjunqq hjunqq commented Jan 9, 2024 •
edited
Loading

Context

fix bug

TS7016: Could not find a declaration file for module BufferObject and Texture

Results

No Errors

Changes

  • Documentation and TypeScript definitions were updated to match those changes
  • TypeScript definitions were updated

PR and Code Checklist

  • semantic-release commit messages
  • Run npm run reformat to have correctly formatted code

Testing

  • This change adds or fixes unit tests
  • Tested environment:
    • vtk.js:
    • OS:
    • Browser:

@floryst
Copy link
Collaborator

floryst commented Jan 10, 2024

Hi, thanks for the contribution!

Your changes define interfaces using the default values rather than the actual types. I would recommend looking at the code for vtkOpenGLTexture and other PRs (e.g. #2976) for examples on how to add new type definitions.

@hjunqq
Copy link
Contributor Author

hjunqq commented Jan 11, 2024

I have made a series of enhancements to vtkOpenGLTexture and vtkOpenGLBufferObject to improve its type safety and readability. Here is an overview of the key changes:

Consistency and Clarity in Type Definitions: Following the precedent set by previous Pull Requests, such as #2976 , I ensured that my type definitions are consistent with the existing code in the project. I introduced new types to more clearly represent some complex structures and concepts.

Enhanced Documentation and Comments: I have added detailed comments and documentation, explaining the new types and the modified sections. This will help other developers understand the purpose of these changes and how to use them.

I believe these changes will enhance the robustness and maintainability of vtkOpenGLTexture and vtkOpenGLBufferObject and look forward to everyone's feedback. Thank you for your time and consideration.

floryst
floryst reviewed Jan 15, 2024
View reviewed changes
Copy link
Collaborator

@floryst floryst left a comment

Choose a reason for hiding this comment

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

Thanks for the update! I think it should be good to go after some changes.

There are some whitespace quirks that would be nice to adjust, but I'll just have prettier enforce formatting on d.ts files in a future PR.

Sources/Common/DataModel/SelectionNode/index.d.ts Outdated
@@ -54,7 +54,7 @@ export interface vtkSelectionNode extends vtkObject {
/**
* This functions is called internally by VTK.js and is not intended for public use.
*/
setProperties(properties: ISelectionNodeProperties);
setProperties(properties: ISelectionNodeProperties):any;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
setProperties(properties: ISelectionNodeProperties):any;
setProperties(properties: ISelectionNodeProperties): boolean;

This is an auto-generated setter, so it returns boolean. (Returns true if the underlying value is actually changed).

Sources/Common/DataModel/SelectionNode/index.d.ts Outdated
@@ -64,7 +64,7 @@ export interface vtkSelectionNode extends vtkObject {
/**
* This functions is called internally by VTK.js and is not intended for public use.
*/
setSelectionList(selectionAttributeIDs: ISelectionNodeProperties);
setSelectionList(selectionAttributeIDs: ISelectionNodeProperties):any;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here: should be boolean

Sources/Rendering/OpenGL/BufferObject/index.d.ts Outdated
Comment on lines 8 to 10
objectType: ObjectType;
context?: WebGLRenderingContext | WebGL2RenderingContext;
allocatedGPUMemoryInBytes: number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

objectType and allocatedGPUMemoryInBytes should be optional, since generally initial values are optional.

Sources/Rendering/OpenGL/BufferObject/index.d.ts Outdated
Comment on lines 16 to 21
type vtkOpenGLBufferObjectBase = Omit<vtkObject, 'set'> & vtkAlgorithm;

/**
* Interface for OpenGL Buffer Object
*/
export interface vtkOpenGLBufferObject extends vtkOpenGLBufferObjectBase {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks to me that vtkOpenGLBufferObject merely extends vtkObject instead of what you have defined here.

Sources/Rendering/OpenGL/Texture/index.d.ts Show resolved Hide resolved
Sources/Rendering/OpenGL/Texture/index.d.ts Outdated
/**
* Interface for OpenGL Texture.
*/
export interface vtkOpenGLTexture extends vtkOpenGLTextureBase {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be vtkOpenGLTexture extends vtkViewNode, based on what vtkOpenGLTexture extends in the codebase.

Sources/Rendering/OpenGL/Texture/index.d.ts Outdated
* Renders the texture within the given render window.
* @param renWin The render window in which to render the texture.
*/
render(renWin:any):void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
render(renWin:any):void;
render(renWin: vtkOpenGLRenderWindow):void;

Sources/Rendering/OpenGL/Texture/index.d.ts Outdated
* Releases the graphics resources used by the texture within the given render window.
* @param renWin The render window whose resources should be released.
*/
releaseGraphicsResources(renWin:any):void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
releaseGraphicsResources(renWin:any):void;
releaseGraphicsResources(renWin: vtkOpenGLRenderWindow):void;

Sources/Rendering/OpenGL/Texture/index.d.ts Outdated
* Sets the OpenGL render window in which the texture will be used.
* @param renWin The render window to set.
*/
setOpenGLRenderWindow(renWin:any):void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
setOpenGLRenderWindow(renWin:any):void;
setOpenGLRenderWindow(renWin: vtkOpenGLRenderWindow):void;

Sources/Widgets/Core/AbstractWidgetFactory/index.d.ts Outdated Show resolved Hide resolved
@hjunqq
Copy link
Contributor Author

hjunqq commented Jan 16, 2024

Thank you for your feedback. I have made the necessary corrections as per your suggestions and also adjusted the spacing issues.

@floryst
Copy link
Collaborator

floryst commented Jan 26, 2024

This is good to go after my most recent comment.

@floryst floryst added this pull request to the merge queue Feb 6, 2024
Merged via the queue into Kitware:master with commit 18acd42 Feb 6, 2024
3 checks passed
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 6, 2024

🎉 This PR is included in version 29.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Automated label label Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@floryst floryst floryst left review comments

Assignees
No one assigned
Labels
released Automated label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hjunqq @floryst