Quake MDL feature #1591
Quake MDL feature #1591
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1591 +/- ##
==========================================
+ Coverage 95.72% 95.76% +0.04%
==========================================
Files 128 130 +2
Lines 10293 10558 +265
==========================================
+ Hits 9853 10111 +258
- Misses 440 447 +7 ☔ View full report in Codecov by Sentry. |
|
@Youva any news on this ? |
1 similar comment
|
@Youva any news on this ? |
|
Hi @Youva ! Looks like you are back. First you may want to rebase on the latest master :) |
|
You are modifying libf3d public API! |
|
Looks like your merge did not go as expected. I'd strongly suggest using rebase instead of merge. Let me know if I can help (on discord or here). |
Youva
force-pushed
the
quake-mdl-feature
branch
from
70ec237 to
988186a
Compare
|
Hi @Youva Please resolve discussions you have adressed and let us know if this is ready for review. |
Also changed the config and added some tests.
Youva
force-pushed
the
quake-mdl-feature
branch
from
001a692 to
4bee8b4
Compare
|
Is this ready for review @Youva ? :) |
|
I think it's ready for a review. |
|
@malespiaut please review if you can :) |
| @@ -155,6 +155,7 @@ f3d_test(NAME TestVTM DATA mb.vtm) | |||
| f3d_test(NAME TestVTK DATA cow.vtk) | |||
| f3d_test(NAME TestNRRD DATA beach.nrrd ARGS -s) | |||
| f3d_test(NAME TestSPLAT DATA small.splat ARGS -osy --up=-Y --point-sprites-size=1) | |||
| f3d_test(NAME TestQuakeMDL DATA zombie.mdl ARGS --animation-index=0 --animation-autoplay=true --animation-time=0.1) | |||
There was a problem hiding this comment.
| f3d_test(NAME TestQuakeMDL DATA zombie.mdl ARGS --animation-index=0 --animation-autoplay=true --animation-time=0.1) | |
| f3d_test(NAME TestQuakeMDL DATA zombie.mdl ARGS --animation-index=0 --animation-time=0.1) |
| "match": ".*(mdl)", | ||
| "options": { | ||
| "up": "+Z", | ||
| "animation-index": "-1" |
There was a problem hiding this comment.
So most animated models have multiples animations baked in and all animations should be enabled by default ?
| "match": ".*(mdl)", | ||
| "options": { | ||
| "up": "+Z", | ||
| "animation-index": "-1" |
There was a problem hiding this comment.
that doesnt make much sense for thumbnail to have a animation related option tbh
| std::string filename = | ||
| std::string(argv[1]) + "data/zombie_2.mdl"; // File was modified to add coverage. | ||
| vtkNew<vtkF3DQuakeMDLImporter> importer; | ||
| std::cout << filename; |
| int TestF3DQuakeMDLImporter(int vtkNotUsed(argc), char* argv[]) | ||
| { | ||
| std::string filename = | ||
| std::string(argv[1]) + "data/zombie_2.mdl"; // File was modified to add coverage. |
There was a problem hiding this comment.
ideally clarify what codepath it covers ?
error ? specific feature ?
| int skinWidth, int skinHeight, int nbSkins, int selectedSkinIndex) | ||
| { | ||
| constexpr int char_size = 1; | ||
| constexpr int int_size = 4; |
| ptr[1] = F3DMDLDefaultColorMap[index][1]; // G | ||
| ptr[2] = F3DMDLDefaultColorMap[index][2]; // B | ||
| } | ||
| } |
There was a problem hiding this comment.
these for loops looks like they could be replace by std::copy calls
| { | ||
| constexpr char char_size = 1; // Size of char in file | ||
| constexpr int int_size = 4; // Size of int in file | ||
| constexpr int float_size = 4; // Size of float in file |
| animationId++; | ||
| frameName = meshName; | ||
| this->NumberOfAnimations++; | ||
| this->AnimationNames.emplace_back(meshName); |
There was a problem hiding this comment.
is there a way to cover this code ?
| //---------------------------------------------------------------------------- | ||
| void UpdateTimeStep(double timeValue) | ||
| { | ||
| int frameIndex = static_cast<int>(floor(FrameRate * abs(timeValue))) % |
There was a problem hiding this comment.
| int frameIndex = static_cast<int>(floor(FrameRate * abs(timeValue))) % | |
| int frameIndex = static_cast<int>(std::floor(FrameRate * std::abs(timeValue))) % |
| std::string Description; | ||
| vtkSmartPointer<vtkPolyDataMapper> Mapper; | ||
| std::vector<vtkSmartPointer<vtkPolyData>> Mesh; | ||
| std::vector<std::pair<int, float>> AnimationIds; |
There was a problem hiding this comment.
std::map<int, float> instead ?
There was a problem hiding this comment.
what does the float represent here ?
| std::vector<std::string> AnimationNames; | ||
| std::vector<int> ActiveFrames; | ||
| std::vector<int> ActiveAnimationIds; | ||
| int NumberOfAnimations = 0; |
There was a problem hiding this comment.
is this needed ? its basically the size of AnimationsIds ?
| bool vtkF3DQuakeMDLImporter::GetTemporalInformation(vtkIdType animationIndex, double frameRate, | ||
| int& nbTimeSteps, double timeRange[2], vtkDoubleArray* vtkNotUsed(timeSteps)) | ||
| { | ||
| this->Internals->SetFrameRate(frameRate); |
There was a problem hiding this comment.
F3D doesnt realy care about framerate and will only use UpdateTimeStep with an actual time in seconds.
There was a problem hiding this comment.
Does Quake MDL only defines frames without an associated time ?
| ///@} | ||
|
|
||
| /** | ||
| * Get temporal information for the currently enabled animation. |
There was a problem hiding this comment.
| * Get temporal information for the currently enabled animation. | |
| * Get temporal information for the currently enabled animations. |
Adds class vtkQuakeMDLImporter that reads .MDL files.
The class :
Fix: #1310