JuliaIO / MeshIO.jl

IO for Meshes
Other
79 stars 31 forks source link

Fix vertex remapping of obj files #64

Closed ffreyer closed 3 years ago

ffreyer commented 3 years ago

Currently

catmesh = FileIO.load(GLMakie.assetpath("cat.obj"));
texture = FileIO.load(GLMakie.assetpath("diffusemap.tga"))
mesh(catmesh, color=texture)

looks wrong because the normal and uv remapping discards points. (Specifically points along the edges of the texture, where one position has two (or more) uv coordinates associated with it.) I fixed that by duplicating the vertices in question and adjusting faces accordingly.

Before: Screenshot from 2020-10-18 01-07-17

After: Screenshot from 2020-10-24 04-13-53

codecov[bot] commented 3 years ago

Codecov Report

Merging #64 (619ff12) into master (0465764) will increase coverage by 0.42%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   96.49%   96.91%   +0.42%     
==========================================
  Files           8        8              
  Lines         342      357      +15     
==========================================
+ Hits          330      346      +16     
+ Misses         12       11       -1     
Impacted Files Coverage Δ
src/io/obj.jl 95.83% <100.00%> (+2.85%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0465764...1d4fd54. Read the comment docs.

ffreyer commented 3 years ago

Did some performance tweaking. A mesh with 120k faces loaded in ~0.65s before this pr, ~12s after the first commit and ~1s after the latest.

ffreyer commented 3 years ago

I noticed that if there is a vertex with indices (1,), (1, 1) (1, 1, 1) etc it gets overwritten by the next vertex with position index 1. I switched to using typemax because of that.

SimonDanisch commented 3 years ago

Oh wow.... And also, completely forgot about this awesome PR... We should just merge it quickly!

ffreyer commented 3 years ago

Same :laughing: I think you asked me on Slack if these changes could be made/moved to GeometryBasics and I had no idea how to do that...

ffreyer commented 3 years ago

If GeometryBasics could handle faces with different indices for positions, uvs and normals this would be unnecessary. I don't know if it can nor how to get there though.

ffreyer commented 3 years ago

Actually there still seems to be something wrong with this. I'm losing a normal somehow...

Nvm