BHoM / BHoM_Engine

Internal manipulation of the BHoM
GNU Lesser General Public License v3.0
26 stars 14 forks source link

Geometry_Engine: strategy for geometrical tolerance #1773

Open pawelbaran opened 4 years ago

pawelbaran commented 4 years ago

Description:

As discussed on the call, at the moment the attitude towards geometrical tolerance is rather relaxed. We have a multitude of cases, I gave myself 15 minutes for a search and found almost 10 of them.

Correct:

Incorrect:

Probably a few more could be found. This looks messy, would be nice to standardize. My idea would be to make every Geometry_Engine method accept tolerance argument with a default value. By these means we will make sure the code is consistent and we will lower the chance of tolerance not being passed downstream (easier to follow a single, clear standard + easier to CI check if every method call contains the argument).

What do you guys think?

kThorsager commented 4 years ago

Some more difficulties and options:

The option for users to use different kinds of tolerances for Numerical, Analytical and Geometrical operations (Not a geometrical example, but say one tries to find the equilibrium by stepping of a structure and uses tolerance to evaluate if the forces equal out and to evaluate distances, which is an extreme case of need of different tolerances) A option to retain the simplicity of a single value and add the option to specify would be a tolerance class which one could specify different tolerances for different kind of operations, which would also include casting to and from double for all the cases where it does not matter.

public class Tol
{
    /***************************************************/
    /**** Properties                                ****/
    /***************************************************/

    public virtual double Geometrical { get; set; } = Tolerance.Distance;

    public virtual double Numerical { get; set; } = Tolerance.Distance;

    public virtual double Analytical { get; set; } = Tolerance.Distance;

    /***************************************************/
    /**** Static Operators Override                 ****/
    /***************************************************/

    public static implicit operator Tol(double tolerance)
    {
        return new Tol
        {
            Geometrical = tolerance,
            Numerical = tolerance,
            Analytical = tolerance,
        };
    }

    /***************************************************/

    public static implicit operator double(Tol tolerance)
    {
        return tolerance.Numerical;
    }

    /***************************************************/

And I'm not super keen on adding tolerance inputs for each method as that would truly make it into that value that no-one cares about as it wouldn't do anything in most cases.

Would rather that only methods which do use it have it as input and those who no not need it are called by the interfaces by this kind of system.

/***************************************************/
/**** Public Methods - Interfaces               ****/
/***************************************************/

public static Point ICentroid(this ICurve curve, double tolerance = Tolerance.Distance)
{
    return Centroid(curve as dynamic, tolerance);
}

/***************************************************/
/**** Private Methods - Interfaces              ****/
/***************************************************/

private static Point Centroid(this ICurve curve, double tolerance)
{
    return Centroid(curve as dynamic);
}

/***************************************************/
/**** Private Methods - Fallback                ****/
/***************************************************/

private static Point Centroid(this ICurve curve)
{
    Engine.reflection.RecordError("error.");
    return null;
}

/***************************************************/

And then I have a slightly more wack suggestion which might both be unfeasible and against the framework, but I'll give it a shot.

Add tolerance as a variable in a static class which would be exposed in the UI like these for each component. That would remove it as an input altogether ensuring that it can't not be passed: image Have a suspicion that it might be hell for any parallelisation works in the future maybe. (Not super set on this last one but figured that it might be worth getting out there)

pawelbaran commented 4 years ago

And I'm not super keen on adding tolerance inputs for each method as that would truly make it into that value that no-one cares about as it wouldn't do anything in most cases.

From what I have seen in the code, I think we could be surprised by the number of methods that should take/pass tolerance but actually do not. So in my opinion tolerance will do something in most cases.

BTW, we need to remember about AngleTolerance as well.

pawelbaran commented 4 years ago

Following today's discussion, in the 1st shot the geometry methods should be refactored to take/pass the tolerance wherever relevant - to be done manually, based on the information extracted using Reflection - @IsakNaslundBh, could you please share the script that you have just created? Thanks!

Concerning the prototype proposed by @kThorsager in the comment above, a dedicated issue has been raised for that on the oM side: https://github.com/BHoM/BHoM/issues/1002 - @LMarkowski FYI.

IsakNaslundBh commented 4 years ago

Thanks @pawelbaran .

Script saved here: https://burohappold.sharepoint.com/:f:/s/BHoM/EhLII8rNpPlJugrdRBSyvnkBkHjiUh1QWk1AhpBJkzgUKg?e=v8NWV3