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

Quake MDL feature #1591

Merged
merged 69 commits into from
Mar 30, 2025
Merged

Quake MDL feature #1591

merged 69 commits into from
Mar 30, 2025

Conversation

Youva
Copy link
Contributor

@Youva Youva commented Aug 25, 2024

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.

Fix: #1310

Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 99.61832% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.84%. Comparing base (9e9123e) to head (c501d96).
Report is 20 commits behind head on master.

Files with missing lines Patch % Lines
plugins/native/module/vtkF3DQuakeMDLImporter.cxx 99.61% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1591      +/-   ##
==========================================
+ Coverage   95.75%   95.84%   +0.09%     
==========================================
  Files         125      127       +2     
  Lines       10572    10834     +262     
==========================================
+ Hits        10123    10384     +261     
- Misses        449      450       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

changes needed

Copy link
Member

@Meakk Meakk left a comment

Choose a reason for hiding this comment

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

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 :)

@mwestphal
Copy link
Member

@Youva any news on this ?

1 similar comment
@mwestphal
Copy link
Member

@Youva any news on this ?

@mwestphal
Copy link
Member

Hi @Youva ! Looks like you are back. First you may want to rebase on the latest master :)

Copy link

You are modifying libf3d public API! ⚠️Please update bindings accordingly⚠️!
You can find them in their respective directories: python, java, webassembly.

@mwestphal
Copy link
Member

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 Youva force-pushed the quake-mdl-feature branch from 70ec237 to 988186a Compare December 18, 2024 22:51
@mwestphal
Copy link
Member

Hi @Youva

Please resolve discussions you have adressed and let us know if this is ready for review.

@Youva Youva force-pushed the quake-mdl-feature branch from 4bee8b4 to 14613da Compare March 24, 2025 00:19
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@mwestphal
Copy link
Member

Hi @Youva , is this rdy for review ?

@Youva
Copy link
Contributor Author

Youva commented Mar 27, 2025

Animations will often skip frames on windows, not sure why.

@mwestphal
Copy link
Member

Animations will often skip frames on windows, not sure why.

Slow to read maybe ?

@mwestphal
Copy link
Member

Hi @Youva

Can we metrge this and improve on it ?
We would like to release soon :)

@Youva
Copy link
Contributor Author

Youva commented Mar 29, 2025

I think everything else is fine.

@mwestphal
Copy link
Member

Hi @Youva

Looks like there are a few review item not adressed yet :)
It's ok, should we take over to adress them ?

Let us know :)

@mwestphal
Copy link
Member

Alright, lets merge this, we will then improve on it iteratively.

Copy link
Member

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

Some changes needed, but we (maintainers) will take of them after this is merged.

@mwestphal mwestphal merged commit d1fddff into f3d-app:master Mar 30, 2025
53 checks passed
@mwestphal
Copy link
Member

Thanks for the contribution @Youva @malespiaut !

Copy link
Member

Choose a reason for hiding this comment

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

@Youva Can you tell me where you found this file ?
I cannot find it in LibreQuake or anywhere else, did you create it from zombie.mdl yourself ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified the file using hxd to cover some lines in the source code, it's basically a change in metadata so I think it's still LibreQuake's file.
I can make a basic mdl file if it's an issue.

Copy link
Member

@mwestphal mwestphal Apr 9, 2025

Choose a reason for hiding this comment

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

Thanks! Its ok to do it like that, no need to do more :)

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.

Add support of Quake 1 MDL files.
4 participants