Quake MDL feature #1591

Youva · 2024-08-25T17:33:14Z

Adds class vtkQuakeMDLImporter that reads .MDL files.

The class :

Reads vertices and triangle data from the format to display a model.
Reads a texture and texture coordinates from the format to display onto the model.
Reads animations, stored as an array of vertices in the file, and saves them as a vector of polydata.
Groups animations and displays them.
Adds light sources to the scene.

…es. Fixed texture coordinates not rendering correctly by adding more vertices. Fixed camera position and lighting, animations display properly but could still be improved.

Adds support for the .mdl format.

Fixed incorrect type at line 452.

Changed some int variables to size_t.

codecov · 2024-09-24T19:08:54Z

Codecov Report

Attention: Patch coverage is 0% with 267 lines in your changes missing coverage. Please review.

Project coverage is 93.58%. Comparing base (5f3745f) to head (98f7e72).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
plugins/native/module/vtkQuakeMDLImporter.cxx	0.00%	261 Missing ⚠️
plugins/native/module/vtkQuakeMDLImporter.h	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1591      +/-   ##
==========================================
- Coverage   96.82%   93.58%   -3.24%     
==========================================
  Files         108      110       +2     
  Lines        7643     7938     +295     
==========================================
+ Hits         7400     7429      +29     
- Misses        243      509     +266

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mwestphal · 2024-09-25T05:43:55Z

plugins/alembic/module/vtkF3DAlembicReader.h

 #include <vtkNew.h>
 #include <vtkPolyDataAlgorithm.h>
 #include <vtkVersion.h>

-#include <memory>
-


no need to move this

mwestphal · 2024-09-25T05:44:11Z

plugins/alembic/module/vtkF3DAlembicReader.h.rej

+-#include <memory>
+
+ class vtkF3DAlembicReader : public vtkPolyDataAlgorithm
+ {


you can remove this file

mwestphal · 2024-09-25T05:44:57Z

style.diff

+-  
+ }
+
+ //----------------------------------------------------------------------------


you can remove this file

mwestphal · 2024-09-25T05:45:36Z

plugins/native/CMakeLists.txt

+f3d_plugin_declare_reader(
+  NAME QuakeMDL
+  EXTENSIONS mdl
+  MIMETYPES application/vnd.mdl


do you have a small .mdl file with a creative commons license ?

mwestphal · 2024-09-25T05:45:51Z

plugins/native/module/vtkQuakeMDLImporter.cxx.rej

+-  
+ }
+
+ //----------------------------------------------------------------------------


you can remove this file

mwestphal · 2024-09-25T05:47:08Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+
+    renderer->AddLight(frontLight);
+    renderer->AddLight(leftLight);
+    renderer->AddLight(backLight);


any specific reason for adding lights ?

same question, I don't think it's needed.

mwestphal · 2024-09-25T05:48:06Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+    { -0.850651f, 0.000000f, -0.525731f }, { -0.688191f, 0.587785f, -0.425325f },
+    { -0.587785f, 0.425325f, -0.688191f }, { -0.425325f, 0.688191f, -0.587785f },
+    { -0.425325f, -0.688191f, -0.587785f }, { -0.587785f, -0.425325f, -0.688191f },
+    { -0.688191f, -0.587785f, -0.425325f } };


what are those big number lists ?

mwestphal · 2024-09-25T05:48:53Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+//----------------------------------------------------------------------------
+std::string vtkQuakeMDLImporter::GetAnimationName(vtkIdType animationIndex)
+{
+  return std::to_string(animationIndex);


animation are not named ?

mwestphal · 2024-09-25T05:49:10Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+//----------------------------------------------------------------------------
+void vtkQuakeMDLImporter::DisableAnimation(vtkIdType vtkNotUsed(animationIndex))
+{
+  return;


missing an implementation

mwestphal · 2024-09-25T05:50:24Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+  }
+
+  // Only one animation enabled at a time
+  void EnableAnimation(vtkIdType animationIndex)


only one animation can be enabled at a time ?

mwestphal · 2024-09-25T05:50:53Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+{
+  this->Superclass::PrintSelf(os, indent);
+  os << indent << "FileName: " << this->FileName << "\n";
+}


you can remove PrintSelf method imo

mwestphal · 2024-09-25T05:51:09Z

plugins/native/module/vtkQuakeMDLImporter.h

+   * Set/Get the file name.
+   */
+  vtkSetMacro(FileName, std::string);
+  vtkGetMacro(FileName, std::string);


you can remove this method

mwestphal · 2024-09-25T05:52:17Z

plugins/native/module/vtkQuakeMDLImporter.h

+  ///@{
+  /**
+   * Enable/Disable/Get the status of specific animations
+   * Only one single animation can be enabled


this is a limitation of the assimp importer, but can it be implemented correctly here ?

mwestphal · 2024-09-25T05:52:31Z

plugins/native/module/vtkQuakeMDLImporter.h

+   * Set/Get collada fixup flag.
+   */
+  vtkSetMacro(ColladaFixup, bool);
+  vtkGetMacro(ColladaFixup, bool);


you can remove this

mwestphal · 2024-09-25T05:53:17Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+    roll->RotateX(270);
+    roll->RotateZ(270);
+    renderer->GetActiveCamera()->SetPosition(cameraPosition);
+    renderer->GetActiveCamera()->SetModelTransformMatrix(roll->GetMatrix());


why adding a camera ?

Probably to correct the actor orientation. Are MDL files +Z up by default? Then you just need to specify it in the config files (improvements of config file is missing by the way).

mwestphal · 2024-09-25T05:53:47Z

plugins/native/module/vtkQuakeMDLImporter.h.rej

+-
+
+   void PrintSelf(ostream& os, vtkIndent indent) override;
+


you can remove this file

mwestphal

changes needed

Meakk

Nice work @Youva !!
There is still some clean-up to do, especially memory leaks to fix, but they will appear in the CI when you will add a test.
90% of the job is done, and there's the annoying, yet important, finalization left :)

Meakk · 2024-09-26T08:42:00Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+    roll->RotateX(270);
+    roll->RotateZ(270);
+    renderer->GetActiveCamera()->SetPosition(cameraPosition);
+    renderer->GetActiveCamera()->SetModelTransformMatrix(roll->GetMatrix());


Probably to correct the actor orientation. Are MDL files +Z up by default? Then you just need to specify it in the config files (improvements of config file is missing by the way).

Meakk · 2024-09-26T08:48:33Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+
+    renderer->AddLight(frontLight);
+    renderer->AddLight(leftLight);
+    renderer->AddLight(backLight);


same question, I don't think it's needed.

Meakk · 2024-09-26T08:49:37Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+  }
+
+  //----------------------------------------------------------------------------
+  vtkSmartPointer<vtkTexture> CreateTexture(std::vector<unsigned char> buffer, int& offset,


Avoid a copy

Suggested change

vtkSmartPointer<vtkTexture> CreateTexture(std::vector<unsigned char> buffer, int& offset,

vtkSmartPointer<vtkTexture> CreateTexture(const std::vector<unsigned char>& buffer, int& offset,

Meakk · 2024-09-26T08:50:44Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+      int group;
+      unsigned char* skin;
+    };
+    mixed_pointer_array* skins = new mixed_pointer_array[nbSkins];


not deleted. But let's use a std::vector instead.

Meakk · 2024-09-26T08:52:00Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+    return texture;
+  }
+
+  void CreateMesh(std::vector<unsigned char> buffer, int offset, mdl_header_t* header)


Suggested change

void CreateMesh(std::vector<unsigned char> buffer, int offset, mdl_header_t* header)

void CreateMesh(const std::vector<unsigned char>& buffer, int offset, mdl_header_t* header)

Meakk · 2024-09-26T08:55:40Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+        std::pair<int, float> pair = std::make_pair(frameIndex, 0.0);
+        GroupAndTimeVal.emplace_back(pair);


Can be simplified to

Suggested change

std::pair<int, float> pair = std::make_pair(frameIndex, 0.0);

GroupAndTimeVal.emplace_back(pair);

GroupAndTimeVal.emplace_back({ frameIndex, 0.0 });

Meakk · 2024-09-26T08:56:42Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+        {
+          for (int i = 0; i < header->numTriangles; i++)
+          {
+            vtkIdType* vertexNum = new vtkIdType[3];


memory leak? Why not using that?

Suggested change

vtkIdType* vertexNum = new vtkIdType[3];

vtkIdType vertexNum[3];

Meakk · 2024-09-26T08:57:33Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+        {
+          double* v_0 = Mesh[i]->GetPoint(j);
+          double* v_1 = Mesh[i + 1]->GetPoint(j);
+          double* interp = new double[3];


leak again

Suggested change

double* interp = new double[3];

double interp[3];

Meakk · 2024-09-26T08:58:44Z

plugins/native/module/vtkQuakeMDLImporter.cxx

+
+  void UpdateFrame(double timeValue)
+  {
+    // Hardcoded frames per second, 60FPS


Is it always 60fps in the MDL specifications?

Meakk · 2024-09-26T09:01:00Z

plugins/native/module/vtkQuakeMDLImporter.h

+  void ImportLights(vtkRenderer*) override;
+
+  // Header definition,
+  struct mdl_header_t


can be moved to the cpp file in the internals

malespiaut and others added 20 commits May 27, 2024 11:02

Adding preliminary Quake1 MDL plugin.

9d96975

Fixing some vector types error in vtkQuakeMDLReader.

31e502a

Fixing some formatting error (whitespaces) in vtkQuakeMDLReader.

614385b

Fixing typo in vtkQuakeMDLReader.

802a5ef

Cleaning out unnecessary code.

bccc126

Add importer

b294c6a

Removed hardcoded path

6f7832f

Fixed crash due to memory allocation on normals and interpolated fram…

803caa9

…es. Fixed texture coordinates not rendering correctly by adding more vertices. Fixed camera position and lighting, animations display properly but could still be improved.

Fix for test on --scan-plugins leading to crash

415e7a5

Merge remote-tracking branch 'origin-f3d/master' into quake-mdl-fork.

3fe48e9

Adds support for the .mdl format.

Removed modification at line 255, engine.cxx

f390b60

Formatting

18e2348

Formatting

b4b49b4

Formatting

7cc7e0c

Fixed build errors w/ strict_build

845a0e6

Update vtkQuakeMDLImporter.cxx

2b9de40

Fixed incorrect type at line 452.

Update vtkQuakeMDLImporter.cxx

e22fe52

Changed some int variables to size_t.

Fixed override updateTimeStep.

71a6bda

Removed some warnings

77f65ee

Applied style.

9b5e644

Youva added 4 commits September 24, 2024 15:23

Changed push_back to emplace_back

c6cba7f

Return bool l. 432

59b17e1

Fix unusedStructMember and containerOutOfBounds

81ad26f

Coding style modification.

98f7e72

mwestphal reviewed Sep 25, 2024

View reviewed changes

style.diff

-

}

//----------------------------------------------------------------------------

Copy link

Contributor

mwestphal Sep 25, 2024

Choose a reason for hiding this comment

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

you can remove this file

mwestphal reviewed Sep 25, 2024

View reviewed changes

mwestphal requested changes Sep 25, 2024

View reviewed changes

Meakk reviewed Sep 26, 2024

View reviewed changes

Meakk mentioned this pull request Sep 26, 2024

Add support of Quake 1 MDL files. #1310

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quake MDL feature #1591

Quake MDL feature #1591

Youva commented Aug 25, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

mwestphal Sep 25, 2024

mwestphal Sep 25, 2024

mwestphal Sep 25, 2024

mwestphal Sep 25, 2024

mwestphal Sep 25, 2024

mwestphal Sep 25, 2024

Meakk Sep 26, 2024

mwestphal Sep 25, 2024

mwestphal Sep 25, 2024

mwestphal Sep 25, 2024

mwestphal Sep 25, 2024

mwestphal Sep 25, 2024

mwestphal Sep 25, 2024

mwestphal Sep 25, 2024

mwestphal Sep 25, 2024

mwestphal Sep 25, 2024

Meakk Sep 26, 2024

mwestphal Sep 25, 2024

mwestphal left a comment

Meakk left a comment

Meakk Sep 26, 2024

Meakk Sep 26, 2024

Meakk Sep 26, 2024

Meakk Sep 26, 2024

Meakk Sep 26, 2024

Meakk Sep 26, 2024

Meakk Sep 26, 2024

Meakk Sep 26, 2024

Meakk Sep 26, 2024

Meakk Sep 26, 2024

	vtkSmartPointer<vtkTexture> CreateTexture(std::vector<unsigned char> buffer, int& offset,
	vtkSmartPointer<vtkTexture> CreateTexture(const std::vector<unsigned char>& buffer, int& offset,

	void CreateMesh(std::vector<unsigned char> buffer, int offset, mdl_header_t* header)
	void CreateMesh(const std::vector<unsigned char>& buffer, int offset, mdl_header_t* header)

		std::pair<int, float> pair = std::make_pair(frameIndex, 0.0);
		GroupAndTimeVal.emplace_back(pair);

	std::pair<int, float> pair = std::make_pair(frameIndex, 0.0);
	GroupAndTimeVal.emplace_back(pair);
	GroupAndTimeVal.emplace_back({ frameIndex, 0.0 });

	vtkIdType* vertexNum = new vtkIdType[3];
	vtkIdType vertexNum[3];

Quake MDL feature #1591

Are you sure you want to change the base?

Quake MDL feature #1591

Conversation

Youva commented Aug 25, 2024 • edited Loading

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwestphal left a comment

Choose a reason for hiding this comment

Meakk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youva commented Aug 25, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading