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

feat(objwriter): add support for vtkOBJWriter #2405

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

Conversation

daker
Copy link
Collaborator

@daker daker commented May 8, 2022

fix #2404

Context

Add support for vtkOBJWriter

Results

Changes

  • Documentation and TypeScript definitions were updated to match those changes

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:

@daker daker force-pushed the add-objwriter branch 2 times, most recently from 68353a6 to 175a919 Compare May 8, 2022 12:37
@jourdain jourdain self-requested a review May 8, 2022 15:14
Copy link
Collaborator

@jourdain jourdain left a comment

Choose a reason for hiding this comment

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

LGTM

/**
* vtkOBJWriter writes wavefront obj (.obj) files in ASCII form. OBJ files
* contain the geometry including lines, triangles and polygons. Normals and
* texture coordinates on points are also written if they exist.
Copy link
Member

Choose a reason for hiding this comment

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

you might want to mention that materials are not written out.

Copy link
Collaborator Author

@daker daker May 9, 2022

Choose a reason for hiding this comment

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

I have written the code for the mlt part, but i don't know how it can be exposed? user need to pass the vtkTexture? or just pass the texture filename?

@jourdain do you have any idea how to expose the texture export?

Copy link
Member

@finetjul finetjul May 9, 2022

Choose a reason for hiding this comment

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

yes, a vtkTexture (or vtkImageData?) would need to be optionnally passed (i.e. as a second input macro.algo(publicAPI, model, 2, 1);)

I guess it could output the mtl part in a second output (i.e. macro.algo(publicAPI, model, 1, 2); and outData[1] = ... )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok i will re-add the mtl code again

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i am trying to finish this PR, by calling the getOutputData(1) i am getting an error

Uncaught TypeError: model.output[count].isDeleted is not a function
    at publicAPI.shouldUpdate (macros.js:853:55)
    at Object.getOutputData (macros.js:839:19)

in both outputs i am returning strings

const objContent = writer.getOutputData(0);
const mtlContent = writer.getOutputData(1);

Displaying the content of model.output[count], i can see it's returned string hence the error, so i am not sure if it's a bug or not.

image

Copy link
Member

Choose a reason for hiding this comment

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

Did you check vtkMoleculeToRepresentation that produces 2 outputs ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes but it does produce two polydatas that's why it works, here i am producing two strings to be returned

Copy link
Member

Choose a reason for hiding this comment

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

I see, I think I got confused at some point. A writer is not really meant to have any output.

getOutputData() is supposed to create a vtkDataSet, in your case getOutput() should be enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i don't think getOutput() exist in vtk.js

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, it's a VTK thing, not VTK.js. My bad. Then I guess you should not have those strings as outputs. Maybe that writer should not have any output at the end of the day finally. A simple get accessor could replace those...

@daker daker marked this pull request as draft May 11, 2022 10:24

global.writer = writer;
global.writerReader = writerReader;
global.fullScreenRenderer = fullScreenRenderer;
Copy link
Member

Choose a reason for hiding this comment

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

can you demonstrate how to write the obj, mtl and texture ?

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.

Feature Request: OBJWriter
4 participants