MeshLodGenerator minor issues

[chilly willy]

I'm working with the MeshLodGenerator. It's awesome but I have a couple minor issues.

1. Lines: It is not compatible with lines. Unbuffered input ignores lines and buffered input crashes on lines. It does not output lines.
   This branch does some refactoring and addresses issue 1 by supporting input and output of lines. It does not yet collapse the lines at reduced lods because that may depend on how issue 2 is resolved.

2. Hole punching: When a vertex at the seam between two submeshes is collapsed, if the source vertex did not already have an edge connecting to the destination vertex in that submesh then the triangle will be discarded:
   https://github.com/OGRECave/ogre/blob/5 ... #L233-L241

   Code: Select all

                   size_t id = findDstID(srcID, triangle->submeshID);
                   if (id == std::numeric_limits<size_t>::max()) {
                       // Not found any edge to move along.
                       // Destroy the triangle.
                       data->mIndexBufferInfoList[triangle->submeshID].indexCount -= 3;
                       output->triangleRemoved(data, triangle);
                       removeTriangleFromEdges(triangle, src);
                       continue;
                   }
   

   This leaves holes in the mesh's surface. This could be prevented by using a different cost calculator but it seems like you should not get holes no matter which cost you use. Maybe before we run the cost calculator we can mark edges as not collapsible if they connect vertices that do not exist in all the same submeshes.

Open to feedback on issue 1 and looking for ideas for issue 2.

[paroj]

I dont know how to solve the issues, but it might help looking at the design/ TODOs of the MeshLOD component:

• viewtopic.php?p=510964#p510964
• http://sajty.elementfx.com/progressivemesh/GSoC2012.pdf

regarding your refactoring:
the MeshLOD component did not get much love regarding internal interface hiding (aka making stuff private).
I tried to make up for it here: https://github.com/OGRECave/ogre/pull/2869
This will give you conflicts now, but otherwise touching anything will break public API, which is a bad thing after the 14.0 release.

On a sidenote: do not get crazy with the interfaces, the typical downstream usage of MeshLOD is through the MeshLodGenerator singleton or even just the OgreMeshUpgrader CLI - not through extending the core algorithms.

[chilly willy]

This will give you conflicts now, but otherwise touching anything will break public API, which is a bad thing after the 14.0 release.

I thought I was very careful to not break the public API. Is there something specific I broke? Maybe we can resolve this before 14.0 in case we think breaking something might be beneficial.

On a sidenote: do not get crazy with the interfaces, the typical downstream usage of MeshLOD is through the MeshLodGenerator singleton or even just the OgreMeshUpgrader CLI - not through extending the core algorithms.

I'm not sure what you mean by get crazy with the interfaces but as far as I know I did not make any changes to the public interfaces. All I was trying to do was:

1. Transparently eliminate code duplication*
2. Transparently support lines in the existing Input and Output Providers

*To eliminate code duplication I introduced an intermediate class called Common to hold the common data and code from the existing classes. I would have preferred to put that in the base class but didn't want to make the virtual interface heavier. (Ah, maybe that's what you mean.) For example what I did in these three commits is pretty much exactly what you did in this commit. Are you saying I should put the common code in the base class instead of introducing an intermediate class? Or something different?

[paroj]

I thought I was very careful to not break the public API. Is there something specific I broke? Maybe we can resolve this before 14.0 in case we think breaking something might be beneficial.

for instance, this technically breaks the API, as code written against the previous definition will not compile:
https://github.com/OGRECave/ogre/commit ... 70cdfb7c57
as it is only used internally, I will hide the header as I did with input/ output providers. This essentially removes it from the public API.

on a sidenote: you can actually store T[] in a shared_ptr: https://en.cppreference.com/w/cpp/memor ... _ptr#Notes
However, those classes can just be replaced with IndexData/ VertexData and DefaultHardwareBuffers, which will further simplify implementation.

(Ah, maybe that's what you mean.) Are you saying I should put the common code in the base class instead of introducing an intermediate class?

yes, exactly

[chilly willy]

for instance, this technically breaks the API, as code written against the previous definition will not compile:
https://github.com/OGRECave/ogre/commit ... 70cdfb7c57
as it is only used internally, I will hide the header as I did with input/ output providers. This essentially removes it from the public API.

Ah I see. Sounds good.

However, those classes can just be replaced with IndexData/ VertexData and DefaultHardwareBuffers, which will further simplify implementation.

Cool, that will make it much cleaner.

I resolved the issues I was having in my existing branch. Now it supports lines, prevents punching holes, and prevents breaking lines.

I'll start a new branch to build on the changes you made.

Should I introduce bool mPreventPunchHoles and mPreventBreakLines in LodConfig to enable these features? Seems like you'd never want holes.

[chilly willy]

Also, @paroj, was the refactoring I did otherwise looking like a good way to get rid of the duplicate code or was that on the wrong track? Don't wanna go through it all again unless it will help. Would it make sense to just use thread-safe buffers for input and output even if not using multi-threading?

[paroj]

Should I introduce bool mPreventPunchHoles and mPreventBreakLines in LodConfig to enable these features? Seems like you'd never want holes.

these options just waste time, if you only have one submesh, right? Then allowing this to turn off makes sense. Also people might want to generate LOD on loading and thus trade off quality for loading speed.

was the refactoring I did otherwise looking like a good way to get rid of the duplicate code

yes, the code looked good besides the interfaces that were not really needed.

Would it make sense to just use thread-safe buffers for input and output even if not using multi-threading

note that the buffers are not thread-safe in any way. They are just CPU copies that are not needed when all is done on the main thread.

Actually they are not needed for multi-threading either, as long as you only lock/ unlock on the main thread. Probably some Ogre bugs prevented this to work properly back when this was implemented.
I suggest to first focus on the code-deduplication and the features you actually need though.