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

Adding preliminary Quake1 MDL plugin. #1441

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

Conversation

malespiaut
Copy link

This pull request is here to let everyone have a clearer look at the current effort to implement support for Quake1 MDL files; and hopefully help more skilled developpers to give their insight and contribute.

This is tied to issue #1310.

@malespiaut
Copy link
Author

I'm writing an extra comment to make everyone extra sure that this PR is not ready for merging with master. This is, for now, to see the code changes and discuss about it.


std::vector<unsigned char> buffer(std::istreambuf_iterator<char>(inputStream), {});

//??? What's a “splat”???
Copy link
Contributor

Choose a reason for hiding this comment

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

In vtkF3DSplatReader, the splats are actually just points with extra information (radius, color, etc...) because they will be drawn as ellipsoids later by F3D.
In your case, you just want to have points and triangles.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, so does that means that I can remove all SetNumberOfTuples() safely?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, take a look at my other comment, you can just remove all the VTK buffers you have created for now.

size_t nbSplats = buffer.size() / splatSize;

// identity ("IDPO"): 4 chars (4 bytes)
vtkNew<vtkUnsignedCharArray> IDPOArray;
Copy link
Contributor

Choose a reason for hiding this comment

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

So you don't really need to create VTK arrays for internal data, it's only useful for data that will be outputted down the pipeline.
I suggest we start by supporting a small subset for now (i.e. geometry only without texturing nor animation)
For now, you can just use the code in the section "Reading a MDL file" of this page: http://tfc.duke.free.fr/coding/mdl-specs-en.html
Later it would be better to clean that up to modern C++ style for readability.
When this is read, you can take a look at the section "Rendering the model", the idea is just to take the frame index 0 (we'll extend it later to support animation), and just transform vertices to a vtkFloatArray (3 components, num_verts tuples) and a vtkIdTypeArray for triangles (1 component, 4*num_tris tuples)
If you're wondering about why "4*num_tris", that's because the buffer is containing the size of the cell before the indices to support quads or polygons with more vertices. Take a look at this code, you should do something similar.

@mwestphal
Copy link
Contributor

an extra comment to make everyone extra sure that this PR is not ready for merging with master. This is, for now, to see the code changes and discuss about it.

Of course! this is fine! You can flag your PR as a draft to make that more clear if you want :)

@malespiaut malespiaut marked this pull request as draft May 30, 2024 07:50
@mwestphal
Copy link
Contributor

hey @malespiaut , need any help moving forward ?

@malespiaut
Copy link
Author

Hello @mwestphal,
No help needed for now, thank you. I'm currently enrolled in a Data Science bootcamp, and have little free time for now.

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.

3 participants