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

exportGraphics should support instancing for symbols inside 3d linestyles #195

Open
dassaf4 opened this issue Mar 8, 2023 · 10 comments
Open
Assignees
Labels
enhancement New feature or request geometry

Comments

@dassaf4
Copy link
Member

dassaf4 commented Mar 8, 2023

Requested to improve performance for Civil tools.

Copied from ADO #796961

@dassaf4 dassaf4 added this to the iTwin.js 4.0 milestone Mar 8, 2023
@dassaf4 dassaf4 self-assigned this Mar 8, 2023
@dassaf4 dassaf4 removed this from the iTwin.js 4.0 milestone Mar 22, 2023
@dassaf4 dassaf4 added the enhancement New feature or request label May 8, 2023
@a-gagnon
Copy link

a-gagnon commented Dec 17, 2024

Hi @dassaf4, I currently have an issue in my hands where the civil backend consumes too much memory and end up crashing the application on mobile. After a bit of investigation, it is caused by 3d linestyles that output many large meshes. The first one I got a hold of outputs 742 meshes with 30k faces each. Just keeping those meshes in memory drains a bunch of resources, and you can imagine processing those is extremely costly.

Would it be possible to give this issue a high priority? It has been reported in a service case by a user (ADO#1450752). As a temporary workaround, I'll either skip elements with linestyles (by inspecting GeometryStream prior to calling exportGraphics).

Dataset(s) are available in the civil work item linked to the service case on ADO. Let me know if I can help.

Thanks!
Alex

@dassaf4
Copy link
Member Author

dassaf4 commented Dec 18, 2024

Hi @a-gagnon, I'll take a look in the new year.

At this point, I have no idea how to approach a solution. It seems that Matt was aware of the problem, with these comments in JsInteropExportGraphics.cpp. @bbastings any pointers for implementing this?

In the meantime, is it possible to strip the 3d linestyle so you at least get the mesh geometry?

@bbastings
Copy link
Contributor

In the meantime, is it possible to strip the 3d linestyle so you at least get the mesh geometry?

If ExportGraphicsBuilder._WantStrokeLineStyle returns false it won't spew the linestyle geometry, which it currently does for non-cosmetic/physical styles.

@a-gagnon
Copy link

a-gagnon commented Dec 19, 2024

In the meantime, is it possible to strip the 3d linestyle so you at least get the mesh geometry?

The workaround is in place -- I just skip the entire element if it has line styles. I don't think I can parameterize just skipping line styles from the typescript side.

At this point, I have no idea how to approach a solution.

From my understanding, most of our problems don't stem from the geometry that gets extruded along the path. It comes from the various GeometryParts being referenced by the line style and outputted as individual meshes rather than GeometryPartInstanceProps like I would expect them.

Here's one problematic element I had to deal with:
Image

I haven't seen the LineStyle definition, but I can safely say that each post, each wooden block behind the rail and each bolt/rivet is a GeometryPart under the hood. It may not look like much, but each bolt of this guardrail is outputted as a mesh with 28k facets.
Image

Using some metrics on each outputted mesh, I went as far as coming up with my own instancing scheme (ie. what GeometryPartInstanceProps would have given me for free).
Image

@bbastings
Copy link
Contributor

Yeah, it's the symbols being exploded. I'm not sure what it would take for export graphics to return 1 mesh with transforms.

Currently the linestyle stroke code does not output the parts as symbols. It could only do this when not using color/weight overrides or clipping. I tried instancing when possible once, it was MUCH slower for tile generation because it didn't instance and ended up exploding them anyway...this bypassed the automatic instancing of identical solid primitive/meshes that normally happens making performance significantly worse. Not sure whether anything has changed there...

So, could be that having export graphics take a similar approach to tile generation and post instance the symbol geometry to not collect such a huge amount of individual meshes would be an option.

Found my code change that tried to output symbols that could be instanced...I'll include it in case someone on the display team wants to revisit this. :)

/*---------------------------------------------------------------------------------**//**
* @bsimethod
+---------------+---------------+---------------+---------------+---------------+------*/
void LsSymbolComponent::Draw(LineStyleContextR context, TransformCR transform, ClipVectorCP clip, bool ignoreColor, bool ignoreWeight)
    {
    bool creatingTexture = context.GetCreatingTexture();

    if (!(creatingTexture || ignoreColor || ignoreWeight || clip))
        {
        Render::GraphicBuilderR mainGraphic = context.GetGraphicR();
        ViewContextR viewContext = context.GetViewContext();
        Render::GeometryParams tmpGeomParams(context.GetGeometryParams());

        viewContext.AddSubGraphic(mainGraphic, m_geomPartId, transform, tmpGeomParams);
        return;
        }

    // NOTE: Unfortunately we can't just call ViewContext::AddSubGraphic for symbols since it won't support things like ignoreColor, ignoreWeight...
    DgnGeometryPartCPtr geomPart = GetGeometryPart();

    if (!geomPart.IsValid())
        return;
...

@bbastings
Copy link
Contributor

As for controlling the linestyle symbol export from typescript. I only see this option...have you tried setting a size?

export interface ExportGraphicsOptions {
  /** The longest dimension of a line style's largest component must be at least this size in order for
   * exportGraphics to evaluate and generate its graphics. If undefined, this defaults to 0.1.
   * Line styles can evaluate to 3D geometry that clients expect to receive from exportGraphics, but they
   * can also generate gigabytes of mesh data when line styles with small components are applied to long
   * line strings.
   */
  minLineStyleComponentSize?: number;

This is how that value is checked/used:

bool _WantStrokeLineStyle(Render::LineStyleSymbCR symb, IFacetOptionsPtr&) override
    {
    if (symb.IsCosmetic()) // Cosmetic are not supportable
        return false;

    ILineStyleCP lineStyle = symb.GetILineStyle();
    if (!lineStyle)
        return false;

    // Clients expect that 3D geometry like guardrails in 3D linestyles will be included. But they also
    // call exportGraphics with very small graphics applied to linestyles that run over many miles, and
    // end up hanging their backend generating enormous graphics of little value. This is an attempt at
    // a default which will include the graphics client expect but not those which cause problems at scale.
    const double defaultMaxDimensionNeededToStroke = 0.1;
    double minDimensionNeededToStroke = m_exportGraphicsProcessor.m_minLineStyleComponentSize;
    if (minDimensionNeededToStroke == 0.0)
        minDimensionNeededToStroke = std::max(m_facetOptions->GetChordTolerance(), defaultMaxDimensionNeededToStroke);

    double maxDimensionInMeters = symb.GetScale() * std::max(lineStyle->GetMaxWidth(), lineStyle->GetLength());
    return maxDimensionInMeters >= minDimensionNeededToStroke;
    }

@a-gagnon
Copy link

I couldn't find minLineStyleComponentSize in our code base so we must default to the 0.1. That might be a good first step to avoid excessive memory consumption.

It's obvious that it'll help the guardrail case, but I have no idea what users might throw at us. A few years back, we had a fire hydrant with roughly 100k facets, and that threshold wouldn't have filtered it out...

From my standpoint, the ideal solution would be to add a flag to the ExportGraphicsOptions while passing the partInstanceArray param.

@bbastings
Copy link
Contributor

bbastings commented Dec 19, 2024

From my standpoint, the ideal solution would be to add a flag to the ExportGraphicsOptions while passing the partInstanceArray param.

I don't disagree, but like I mentioned, that requires the changes to the line style stroke code posted previously and sorting out its issues.

For now, setting a very large size to skip stroking all line styles has to be better than skipping the entire element?

@a-gagnon
Copy link

For now, setting a very large size to skip stroking all line styles has to be better than skipping the entire element?

Yeah, I can look into that. Thanks.

@bbastings
Copy link
Contributor

bbastings commented Dec 19, 2024

@a-gagnon @dassaf4 I created a branch that may help address this.

This should let ExportGraphics accumulate linestyles symbols as parts when part instances are requested (assuming I've understood the ExportGraphics code correctly). If part information isn't requested symbols will continue to be exploded to geometry. I don't think we need another option to control this, not sure why you would want parts and not want symbols treated as part when possible.

From what I've seen with guardrail 3d linestyles the past, the symbols aren't clipped and they aren't ignoring color or weight, so they should be acceptable for the instancing check I added.

I'll leave it to you folks to figure out how to test this.

A few things that should be investigated regardless. Are the facet tolerances reasonable, are the tolerances properly accounting for a part scale? Those facet counts seem excessive for a bolt. If the tolerances are okay, is some sort of decimation warranted then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request geometry
Projects
None yet
Development

No branches or pull requests

3 participants