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

Crack stitching #372

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Eideren
Copy link
Contributor

@Eideren Eideren commented Dec 7, 2022

Crack Stitching

Introduces a toggle to the model inspector UI which, once toggled on, fixes cracks appearing through generated geometry. It does so asynchronously to prevent interrupting the user while he's working.

See discussion on discord for further information.

Inspector

image

Result

image
image

Here's a more advanced implementation of the debugger interface if someone wants to debug this in the future:
StitchingDebugger.zip
Don't forget to comment out #define YIELD_SUBSTEPS above CracksStitching.cs

(cherry picked from commit 263a02f7aac879c6bfabf6df57fbc64c568bc7c5)
Comment on lines +182 to +198
foreach (var (x, y, z) in trianglesToAdd)
{
var (subMeshIndex, mappingA) = posToSubMesh[vertices[x]];
var ( _, mappingB) = posToSubMesh[vertices[y]];
var ( _, mappingC) = posToSubMesh[vertices[z]];

// Effectively randomly picking subMesh, i.e.: material, those triangles will be assigned to, see comment above
var indices = subMeshes[subMeshIndex];

// Not sure yet how to properly solve winding order, add both sides for now
indices.Add(mappingA);
indices.Add(mappingB);
indices.Add(mappingC);
indices.Add(mappingC);
indices.Add(mappingB);
indices.Add(mappingA);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small issue right now is the fact that I'm not yet sure how to solve triangle winding order, so I'm adding triangles facing both sides. If you have a suggestion I am open to implement it.

@nukeandbeans
Copy link
Collaborator

If I am not mistaken, these would technically be called "T-Junctions", if they are vertices along an edge like -|. If this is the case, then changing the title and label would be great.

Also just a minor nitpick: an image comparison with wireframe on would help a bit with making it more apparent exactly what is being done.

Thanks for this! I'm sure a few people would find this very useful (and it would probably fix some PostFX issues, like bloom, for some projects).

@Eideren
Copy link
Contributor Author

Eideren commented Dec 8, 2022

This issue doesn't appear only around t-junctions, see this case where multiple white pixels peek through two edges that should be touching:

It's not caused by the rasterization process either as getting closer does show a multi-pixel wide crack.

an image comparison with wireframe on would help a bit with making it more apparent exactly what is being done.

Sure, edited the post to add one in.

Also, the logs for how long auto lightmap gen and stitching took are a bit too spam-y, could we do something about that ?
Maybe log a warning for auto lightmap gen if it takes a significant amount of time since that's a blocking operation and users might become frustrated waiting for editor response when moving things around. Stitching isn't really blocking so not sure there even is a need to log it ?

(cherry picked from commit 5df561435376dcd80df2702a75ead19e2d16af20)
@Eideren
Copy link
Contributor Author

Eideren commented Dec 10, 2022

Regarding my last complaint about duration logging, I replaced debug logging by unity's progress API.
image
image

@Gawidev
Copy link
Contributor

Gawidev commented Dec 19, 2022

Currently in a very good spot, this feature is really amazing and RCSG will benefit greatly from it.
Only small issue discovered when playing around with it is relating to surfaces set to non-visible, they currently seem to get in the way of the crack detection doing its job properly, sometimes filling them up in a broken manner.

@Eideren
Copy link
Contributor Author

Eideren commented Dec 23, 2022

I'm still looking into @Gawidev's issue, I have a fix - just have to iron out a couple of bugs and with the holidays and all I don't have a lot of time to go through it.

@Eideren
Copy link
Contributor Author

Eideren commented Jan 8, 2023

There we go, this commit should fix those issues.
PR ready for further review/merge.

Copy link
Collaborator

@nukeandbeans nukeandbeans left a comment

Choose a reason for hiding this comment

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

Usings should be outside of any namespace. Additionally, using static will not compile on versions of Unity that don't have the C# 6 update.

Edit: It seems like GH decided I didn't need to quote specific code. This is in the CracksStitching.cs file.

if(bestMatch.dist <= maxDist)
throw new InvalidOperationException($"Stray edge ({thisEdge}) could not be solved for");

debug?.LogWarning($"For edge {thisEdge}, closest match ({bestMatch.edge}) did not satisfy {nameof(maxDist)} constraint {bestMatch.dist}/{maxDist}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be using String.Combine instead of string interpolation, as older versions of Unity do not support it.
Additionally, .? is not supported, so if() checks should be used instead.

foreach (var (x, y, z) in trianglesToAdd)
{
var (subMeshIndex, mappingA) = posToSubMesh[vertices[x]];
var ( _, mappingB) = posToSubMesh[vertices[y]];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Anonymous variables aren't supported on versions previous to C# 6.

Copy link
Collaborator

@nukeandbeans nukeandbeans left a comment

Choose a reason for hiding this comment

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

Everything looks OK. Just need to make sure that there aren't any unsupported API or C# features on older versions of Unity, until the oldest supported version by RCSG (5.6.4f1) is updated to 2019+. I didn't tag every single change, but its easy to find by checking changes on here.

@Gawidev
Copy link
Contributor

Gawidev commented Jan 9, 2023

Hey sorry resident "I-break-stuff" worker here, a bit of extra testing done and I've run into a similar issue where hidden faces are being treated strangely by the crack detection, this case particularly when there are hidden faces with different shadow-receiving and shadow-casting settings.

Should be reproducible by creating a box brush, then setting one surface to non-visible, and another one to non-visible + non-shadow-casting.

They seem to get merged into the same generated render mesh, which shouldn't be the case, iirc there should be up to 5 possible generated render meshes for the same material.
(vis/receiver/caster, vis/receiver, vis/caster, vis only, invis/caster)
(This is cause rcsg assumes hidden faces should only be taken into consideration if they cast shadows)

@nukeandbeans nukeandbeans added this to Pending in WIP via automation Jan 10, 2023
@nukeandbeans
Copy link
Collaborator

Any updates? Would love to get around to merging this.

@Eideren
Copy link
Contributor Author

Eideren commented Aug 3, 2023

I should have some time to take a look at this again this week end, I'll keep you posted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
WIP
  
Pending
Development

Successfully merging this pull request may close these issues.

None yet

3 participants