vsg-dev / VulkanSceneGraph

Vulkan & C++17 based Scene Graph Project
http://www.vulkanscenegraph.org
MIT License
1.32k stars 212 forks source link

ComputeBounds returns with incorrect bounds #1112

Closed rolandhill closed 8 months ago

rolandhill commented 8 months ago

Hi Robert,

I use ComputeBounds to implement a 'zoom to object' function. This always used to work, but has stopped since I upgraded VSG a week or so ago. After calling accept(), the bounds structure does contain data, but it is not correct. The subbranch I'm running it on is under a MatrixTransform that translates everything by the centre of the original data, with the centre having been subtracted from all the vertices (tested and working prior to the update). The vertices are in a VertexIndexDraw. I have other subbranches that are still working, however for all of these I intercept the ComputeBounds visitor in my own accept() override.

My first suspicion is that it is related to https://github.com/vsg-dev/VulkanSceneGraph/issues/1102

Could something have changed in recent updates?

Regards, Roland

rolandhill commented 8 months ago

Just adding, the geometry that I'm trying to zoom to is loaded from dxf with vsgXchange. If I load the same file with vsgViewer, then it works and the viewer zooms to the file correctly. The coordinates are a long way from 0, so I know that the camera has moved. The difference here is that the loaded geometry is not offset by the centre under a MatrixTransform in vsgViewer. It's the matrix that ComputeBounds looks to be struggling with.

Roland

robertosfield commented 8 months ago

There was a ArrayState bug introduced when I merged the Animation work last work, this affected intersections. I tracked down and fixed the bug 5 days ago so if you haven't updated since then please try update to latest VSG master and then retesting.

The commit with the fix is: 1380b6ab6cf079d2ae3ff1e2185cd07e258d7ac9

rolandhill commented 8 months ago

Yes, my version of VSG includes the commit that fixed the ArrayState bug.

I've made a bit more progress on this now. Rather than a problem with ComputeBounds, it appears that the visitor that subtracts the centre from the vertices is having no effect. This code hasn't changed and used to work. To test, I'm using the following to print the bounds before and after subtracting the centre from the vertices (note that 'node' is returned from a vsg::read_cast Assimp loader). The MatrixTransform has been removed for the purposes of testing and the only modification is the TranslateVertices shown below:

vsg::ComputeBounds computeBounds;
node->accept(computeBounds);

std::cout << "CB Min: " << computeBounds.bounds.min << "    Max: " << computeBounds.bounds.max << std::endl;

vsg::dvec3 centre = (computeBounds.bounds.min + computeBounds.bounds.max) * 0.5;

std::cout << "\nTranslating" << std::endl;
auto tv = TranslateVertices::create(-centre);
node->accept(*tv);

vsg::ComputeBounds cb2;
node->accept(cb2);
std::cout << "CB Min: " << cb2.bounds.min << "    Max: " << cb2.bounds.max << std::endl;

with the method in TranslateVertices that does the work being

void TranslateVertices::apply(vsg::VertexIndexDraw& vid)
{
    if(vid.arrays.size() > 0)
    {
        std::cout << "TranslateVertices::apply(vsg::VertexIndexDraw& vid)" << std::endl;

        auto t = vsg::vec3(_translate[0], _translate[1], _translate[2]);

        vsg::vec3Array* varr = reinterpret_cast<vsg::vec3Array*>(vid.arrays[0]->data.get());

        for(auto& v : *varr)
        {
            std::cout << v << std::endl;
            v = v + t;
            std::cout << v << std::endl;
        }
    }
}

The output is

CB Min: 638945 6.05684e+06 -1817.29 Max: 641850 6.05974e+06 1087.68

Translating TranslateVertices::apply(vsg::VertexIndexDraw& vid) 639924 6.0589e+06 -350 -473.812 614 14.8076 641116 6.05817e+06 -350 719.062 -119 14.8076 641285 6.05844e+06 -733.022 887.312 155 -368.215 640092 6.05918e+06 -733.022 -305.625 887.5 -368.215 TranslateVertices::apply(vsg::VertexIndexDraw& vid) 639512 6.05932e+06 75 -885.25 1029 439.808 639698 6.05911e+06 75 -699.5 826 439.808 639735 6.05915e+06 -11.6025 -662.625 860 353.205 639549 6.05935e+06 -11.6025 -848.375 1062.5 353.205 TranslateVertices::apply(vsg::VertexIndexDraw& vid) 639510 6.05822e+06 -285 -887.375 -72.5 79.8076 640500 6.05722e+06 -285 102.625 -1062.5 79.8076 640712 6.05744e+06 -804.615 314.75 -850.5 -439.808 639722 6.05843e+06 -804.615 -675.188 139.5 -439.808 CB Min: 638945 6.05684e+06 -1817.29 Max: 641850 6.05974e+06 1087.68

From this, you can see that the first ComputeBounds calculates the bounds correctly. Modification of the vertices appears to work, but the second ComputeBounds gets the same result as the first one. I'm thinking that my TranslateVertices visitor is now working on a COPY of the arrays, whereas it used to be working on the actual arrays.

However ...

If I run the TranslateVertices a second time, so modify the code above with

  auto tv = TranslateVertices::create(-centre);
  node->accept(*tv);

  std::cout << "\nTranslating second run" << std::endl;
  node->accept(*tv);

Then printing the before and after coordinates shows that the vertices are being successfully modified twice as you would expect (ie they are going from their original position to centred around 0, to the same distance out in the negative direction), but the second ComputeBounds still gives the same values as the first. The output is:

CB Min: 638945 6.05684e+06 -1817.29 Max: 641850 6.05974e+06 1087.68

Translating TranslateVertices::apply(vsg::VertexIndexDraw& vid) 639924 6.0589e+06 -350 -473.812 614 14.8076 641116 6.05817e+06 -350 719.062 -119 14.8076 641285 6.05844e+06 -733.022 887.312 155 -368.215 640092 6.05918e+06 -733.022 -305.625 887.5 -368.215 TranslateVertices::apply(vsg::VertexIndexDraw& vid) 639512 6.05932e+06 75 -885.25 1029 439.808 639698 6.05911e+06 75 -699.5 826 439.808 639735 6.05915e+06 -11.6025 -662.625 860 353.205 639549 6.05935e+06 -11.6025 -848.375 1062.5 353.205 TranslateVertices::apply(vsg::VertexIndexDraw& vid) 639510 6.05822e+06 -285 -887.375 -72.5 79.8076 640500 6.05722e+06 -285 102.625 -1062.5 79.8076 640712 6.05744e+06 -804.615 314.75 -850.5 -439.808 639722 6.05843e+06 -804.615 -675.188 139.5 -439.808

Translating second run TranslateVertices::apply(vsg::VertexIndexDraw& vid) -473.812 614 14.8076 -640871 -6.05767e+06 379.615 719.062 -119 14.8076 -639678 -6.05841e+06 379.615 887.312 155 -368.215 -639510 -6.05813e+06 -3.40704 -305.625 887.5 -368.215 -640703 -6.0574e+06 -3.40704 TranslateVertices::apply(vsg::VertexIndexDraw& vid) -885.25 1029 439.808 -641283 -6.05726e+06 804.615 -699.5 826 439.808 -641097 -6.05746e+06 804.615 -662.625 860 353.205 -641060 -6.05743e+06 718.013 -848.375 1062.5 353.205 -641246 -6.05722e+06 718.013 TranslateVertices::apply(vsg::VertexIndexDraw& vid) -887.375 -72.5 79.8076 -641285 -6.05836e+06 444.615 102.625 -1062.5 79.8076 -640295 -6.05935e+06 444.615 314.75 -850.5 -439.808 -640083 -6.05914e+06 -75 -675.188 139.5 -439.808 -641073 -6.05815e+06 -75 CB Min: 638945 6.05684e+06 -1817.29 Max: 641850 6.05974e+06 1087.68

So, I'm completely confused now.

Could this be a result of any of the recent changes? As I say, it used to work.

Roland

robertosfield commented 8 months ago

Without debugging the code + data you have I can only speculate what might be amiss.

At this point my best guess is perhaps your have cull/LOD nodes in the scene graph such that that the vsg::ComputeBounds is using those cull/LOD nodes bounding sphere for computing the extents rather than traversing the whole scene graph below them and computing the bounds on a per vertex basis.

ComputeBounds has flag for toggling on/off this behavior:

    /// Use the bounding volumes of Cull/LOD nodes etc to compute the bounds rather than traverse their subgraphs.
    /// Using the bounding volumes is faster but may result in less tight bounds around the geometry in the scene.
    bool useNodeBounds = true;

Try setting computeBouds.useNodeBounds to false and see what results you get. This feature hasn't been introduced recently though so I wouldn't have thought it would be related to the Animation work.

robertosfield commented 8 months ago

@rolandhill have you looked at setting computeBouds.useNodeBounds to false?

If this doesn't resolve the issue then we'll need to create a test dataset/example that illustrates the problem so I can investigate further.

rolandhill commented 8 months ago

Hi Robert,

Yes, that did fix the problem, thank you. I've been trying to figure out why it has started happening and it looks like there must be have been a change in vsgXchange - maybe there is a LOD or CULL node being inserted that wasn't there before. I rewrote my TranslateVertices visitor to shift the bounds on LOD / Culls and then it works without setting useNodeBounds to false, but this isn't a perfect solution because you could end up shifting bounds multiple times. I need to go through and find the change to understand it properly (or live with just setting useNodeBounds to false for everything).

I'm travelling at the moment and haven't been able to spend time on it. I should get to it next week.

Regards, Roland

On Wed, 6 Mar 2024 at 04:03, Robert Osfield @.***> wrote:

@rolandhill https://github.com/rolandhill have you looked at setting computeBouds.useNodeBounds to false?

If this doesn't resolve the issue then we'll need to create a test dataset/example that illustrates the problem so I can investigate further.

— Reply to this email directly, view it on GitHub https://github.com/vsg-dev/VulkanSceneGraph/issues/1112#issuecomment-1980391039, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPOEQYDU5Q527HYDNI4QHTYW3LW5AVCNFSM6AAAAABEC2WM3SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBQGM4TCMBTHE . You are receiving this because you were mentioned.Message ID: @.***>

robertosfield commented 8 months ago

The modifications to vsgXchange for reading animation data from assimp included the addition of decorating the created subgraph with a vsg::CulNode so perhaps that's the difference. Do you load the data from vsgXchange::assimp?

rolandhill commented 8 months ago

Yes, it's loaded via Assimp, so that sounds like the problem. If the CullNode is at the top then I could just change that one and ignore any lower CullNodes.

On Wed, 6 Mar 2024, 09:25 Robert Osfield, @.***> wrote:

The modifications to vsgXchange for reading animation data from assimp included the addition of decorating the created subgraph with a vsg::CulNode so perhaps that's the difference. Do you load the data from vsgXchange::assimp?

— Reply to this email directly, view it on GitHub https://github.com/vsg-dev/VulkanSceneGraph/issues/1112#issuecomment-1980988789, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPOEQ7NLEAGF2BBPH6NHTTYW4ROXAVCNFSM6AAAAABEC2WM3SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBQHE4DQNZYHE . You are receiving this because you were mentioned.Message ID: @.***>

robertosfield commented 8 months ago

You can control whether the vsgXchange::assimp loaders inserts cull nodes via the culling option that was added as part of the Animation work:

https://github.com/vsg-dev/vsgXchange/blob/master/include/vsgXchange/models.h#L61

Present the vsgXchange::assimp loader only inserts one CullNode above the loaded subgraph, it doesn't insert any within the loaded sub graph.

rolandhill commented 8 months ago

Thanks, Robert. I've decided to leave the CullNode in place and just modify it's bounds. Good to know where it is and that there is only one inserted.

Cheers, Roland

robertosfield commented 8 months ago

Why do you need to modify it's bounds?

rolandhill commented 8 months ago

The imported data is geospatial and usually in UTM coordiantes, so numbers between 100,000 and 10,000,000. There are 2 scenarios: 1) The viewer is in the same coordinate reference system as the data, in which case I have to calculate the centre, subtract the centre from all the vertices, then put it all under a MatrixTransform. This makes sure there is no z fighting / artefacts. 2) The data is in a different CRS, so I have to convert all the vertices to the CRS of the viewer (which might also be geocentric rather than projected), then basically apply scenario 1).

If there is a CullNode in between the MT and the VID, then the CullNode bounds will not reflect the relative location of the vertices after I've altered them. I thought the simple solution would be to also subtract the centre from the CullNode bounds. As it happens, this doesn't appear to work (I thought it was working, but I hadn't switched back to the vsgXchange master). Now that I've thought it through some more, I can't be sure that this would be appropriate if I have to transform coordinates between UTM - Lat Long - ECEF anyway. The best solution is probably to set "culling" to false for the importer and just recalculate the bounds after I've done any transformations. This is the way I was doing it before anyway.

There's also normal rotations to be done if the data needs to be translated between different CRS.

Roland

On Wed, 6 Mar 2024 at 14:22, Robert Osfield @.***> wrote:

Why do you need to modify it's bounds?

— Reply to this email directly, view it on GitHub https://github.com/vsg-dev/VulkanSceneGraph/issues/1112#issuecomment-1981613044, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPOEQZOZMXRULI3AYSQKZLYW5UHTAVCNFSM6AAAAABEC2WM3SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBRGYYTGMBUGQ . You are receiving this because you modified the open/close state.Message ID: @.***>

robertosfield commented 8 months ago

What data types are you importing via vsgXchange::assimp?

I have worked on the assumption that assimp would be used for non geospatial data, so all with a local coordinate system. If the source vertices are all being imported as floats then the numerical errors would be pretty high.

The VSG itself handles these types of issue be keeping vertex data in a local coordinate frame then uses the double matrices on the camera and internal matrices to manage camera movement and placement of subgraphs in larger coordinate frames.

rolandhill commented 8 months ago

Hi Robert,

Apologies for the delay, I'm travelling for work.

The Assimp imported files can be a range of formats because they are exported from a variety of specialist applications. Unfortunately, dxf remains very common. It's also common for the files to contain no information apart from geometry.

I use the double matrices to transform data into local coordinates, but as you say, if they are coming out of the loader as floats then they will have already lost resolution.

What do you suggest? Would it be possible to have an option in vsgXchange to insert a MatrixTransform internally?

I've attached a small DXF file that uses typical UTM coordinates. It just shows a couple of blob shapes.

Cheers, Roland

TS_Iso_susc_0.4.zip

robertosfield commented 8 months ago

I am away on trip now too!

I don't know the status of double vertex support in Assimp, have a look to see if it's able to read the DXF data as doubles, if it is then we could potentially map this to double vertex data when the VSG builds it's meshes. Doubles are really suitable for good rendering performance but as an intermediate that could be post processed to be float vertex data with a local origin decorated by a MatrixTransform would be fine.

If Assimp isn't capable then perhaps a vsgXchange::DXF implementation would be the best way to honour the original data as much as possible.

Over time I do expect us to occasionally opt to implement 3rd party file format support directly rather than via Assimp, simple to getter a better 1:1 mapping between source data and scene graph. Potentially you could also reuse data directly rather than adapting as well.