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

vtkMapper.setResolveCoincidentTopologyPolygonOffsetParameters errors #3131

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PaulHax
Copy link
Collaborator

@PaulHax PaulHax commented Sep 30, 2024

Context

When calling the static mapper vtkMapper.setResolveCoincidentTopologyPolygonOffsetParameters, there is an error because the "class" does not have a publicAPI with a modified() function.

Results

Does not error now when calling vtkMapper.setResolveCoincidentTopologyPolygonOffsetParameters.

Some other tests are failing after adding the test in this PR. Seems that calling setResolveCoincidentTopologyPolygonOffsetParameters with the initial results of getResolveCoincidentTopologyPolygonOffsetParameters does not completely undo the effect?
image

Changes

The error fix involved adding a noOp modified function. Alternatives could be to check if publicAPI has modified in macros.js or upgrade the Mapper static object with macros.js' function obj(publicAPI = {}, model = {}) "constructor."

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:

@PaulHax
Copy link
Collaborator Author

PaulHax commented Sep 30, 2024

I guess the open questions/todos on this issue/PR:

  • Do we want to keep supporting the vtkMapper.setResolveCoincidentTopologyPolygonOffsetParameters function? We are using it in VolView: https://github.com/Kitware/VolView/blob/main/src/main.js#L34
  • Is this "fix" acceptable: adding the noOp modified function?
  • How to test vtkMapper.setResolveCoincidentTopologyPolygonOffsetParameters without affecting other tests?

@finetjul
Copy link
Member

finetjul commented Oct 2, 2024

Do we want to keep supporting the vtkMapper.setResolveCoincidentTopologyPolygonOffsetParameters function? We are using it in VolView: https://github.com/Kitware/VolView/blob/main/src/main.js#L34

I think so, there is also a Static ResolveCoincidentTopology in VTK C++.

Copy link
Member

@finetjul finetjul left a comment

Choose a reason for hiding this comment

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

Shouldn't you expose a "static" MTime ? so that it can be considered when deciding to building/updating the shader ?

@PaulHax
Copy link
Collaborator Author

PaulHax commented Oct 2, 2024

Shouldn't you expose a "static" MTime ? so that it can be considered when deciding to building/updating the shader ?

I think that makes sense as the global/static factor/offset are added with instance values as a uniform for the mappers shaders. How to do that? Upgrade the static object to be a full macros.obj, then each mapper registers a onModified for the static object and updates its own MTime when the static object changes?

How to test vtkMapper.setResolveCoincidentTopologyPolygonOffsetParameters without affecting other tests?

Do you have any insight on that?

@finetjul
Copy link
Member

finetjul commented Oct 2, 2024

I think that makes sense as the global/static factor/offset are added with instance values as a uniform for the mappers shaders. How to do that? Upgrade the static object to be a full macros.obj, then each mapper registers a onModified for the static object and updates its own MTime when the static object changes?

Yes, that makes sense

@finetjul
Copy link
Member

finetjul commented Oct 2, 2024

How to test vtkMapper.setResolveCoincidentTopologyPolygonOffsetParameters without affecting other tests?
Do you have any insight on that?

I'm not sure what the issue is TBH

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.

2 participants