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

Integrate Std_MeasureDistance into unified measurement facility #13810

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

hlorus
Copy link
Contributor

@hlorus hlorus commented May 3, 2024

Currently the Std_MeasureDistance is still present as it's functionality wasn't completly covered by UMF's MeasureDistance. This was because the UMF version would only measure between actual geometry elements rather than the locations where the user is clicking.

This PR adds a new measure type "MeasureDistanceDetached" to UMF which allows to measure a distance between two locations on the surface of objects.
Also the old command "Std_MeasureDistance" along with the feature "App/MeasureDistance.h" and it's viewprovider "Gui/MeasureDistance.h" get removed here.

Closes #13712

@github-actions github-actions bot added the Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD label May 3, 2024
@adrianinsaval
Copy link
Member

If I'm not mistaken the old command added a document object in the tree which was saved with the document, what happens when opening old files that have these objects? should be looking into some sort of migration code? @wwmayer do you know how we can migrate old objects to a new object type? is it worth it in this case?

@wwmayer
Copy link
Contributor

wwmayer commented May 7, 2024

what happens when opening old files that have these objects?

A message will be printed that the object couldn't be restored.

do you know how we can migrate old objects to a new object type?

A migration is not easily possible because there is no mechanism that the document could ask to create an alternative object if the MeasureDistance cannot be instantiated.
A workaround could be to implement a special Type object for MeasureDistance that instantiates the corresponding feature type of the Measure module. This feature then must handle the properties of MeasureDistance.

@hlorus
Copy link
Contributor Author

hlorus commented May 7, 2024

what happens when opening old files that have these objects?

A message will be printed that the object couldn't be restored.

do you know how we can migrate old objects to a new object type?

A migration is not easily possible because there is no mechanism that the document could ask to create an alternative object if the MeasureDistance cannot be instantiated. A workaround could be to implement a special Type object for MeasureDistance that instantiates the corresponding feature type of the Measure module. This feature then must handle the properties of MeasureDistance.

Could we also keep the old object around and migrate it in the onDocumentRestored method similar to how it's documented here: https://wiki.freecad.org/Scripted_objects_migration ?
The old document could then be removed sometime in the future...

@wwmayer
Copy link
Contributor

wwmayer commented May 7, 2024

Could we also keep the old object around and migrate it in the onDocumentRestored method similar to how it's documented here: https://wiki.freecad.org/Scripted_objects_migration ?

No, this will cause serious problems:
onDocumentRestored would have to instantiate the new object, migrate the properties and then destroy itself. At the time when this method is used the document iterates over its objects and the iterator will become invalid. Another problem is that the pointer the document uses to reference this object will become dangling.

The class MeasureDistance is part of the core system and it would directly import an extension module that is considered bad practise. Lower level modules must not directly depend anyhow on higher level modules as this breaks the design.

@adrianinsaval
Copy link
Member

the question is now is it worth trying to support those objects? is anybody using them?

@wwmayer
Copy link
Contributor

wwmayer commented May 10, 2024

I guess it's used rather rarely. But nevertheless with this trick the object can be restored. Add the following code snippets to Mod/Measure/MeasureDistance.h/cpp:

class MeasureDistanceType: public Base::BaseClass
{
public:
    static Base::Type getClassTypeId();
    Base::Type getTypeId() const override;
    static void init();
    static void* create();

private:
    static Base::Type classTypeId;
};

and

Base::Type MeasureDistanceType::getClassTypeId()
{
    return Base::Type::badType();
}

Base::Type MeasureDistanceType::getTypeId() const
{
    return Base::Type::badType();
}

void MeasureDistanceType::init()
{
    initSubclass(MeasureDistanceType::classTypeId,
                 "App::MeasureDistance",
                 "App::DocumentObject",
                 &(MeasureDistanceType::create));
}

void* MeasureDistanceType::create()
{
    return new MeasureDistance();
}

Base::Type MeasureDistanceType::classTypeId = Base::Type::badType();

In order to read in the properties P1 and P2 MeasureDistance must override handleChangedPropertyName:

void MeasureDistance::handleChangedPropertyName(Base::XMLReader &reader,
                                                const char * TypeName,
                                                const char *PropName)
{
    if (strcmp(PropName, "P1") == 0 && strcmp(TypeName, "App::PropertyVector") == 0) {
        Position1.Restore(reader);
    }
    else if (strcmp(PropName, "P2") == 0 && strcmp(TypeName, "App::PropertyVector") == 0) {
        Position2.Restore(reader);
    }
}

@maxwxyz
Copy link
Collaborator

maxwxyz commented May 14, 2024

@hlorus are you adding the snippets above?

@hlorus
Copy link
Contributor Author

hlorus commented May 14, 2024

@hlorus are you adding the snippets above?

Yeah sure, soonish

@hlorus
Copy link
Contributor Author

hlorus commented May 15, 2024

I guess it's used rather rarely. But nevertheless with this trick the object can be restored. Add the following code snippets to Mod/Measure/MeasureDistance.h/cpp:

class MeasureDistanceType: public Base::BaseClass
{
public:
    static Base::Type getClassTypeId();
    Base::Type getTypeId() const override;
    static void init();
    static void* create();

private:
    static Base::Type classTypeId;
};

and

Base::Type MeasureDistanceType::getClassTypeId()
{
    return Base::Type::badType();
}

Base::Type MeasureDistanceType::getTypeId() const
{
    return Base::Type::badType();
}

void MeasureDistanceType::init()
{
    initSubclass(MeasureDistanceType::classTypeId,
                 "App::MeasureDistance",
                 "App::DocumentObject",
                 &(MeasureDistanceType::create));
}

void* MeasureDistanceType::create()
{
    return new MeasureDistance();
}

Base::Type MeasureDistanceType::classTypeId = Base::Type::badType();

In order to read in the properties P1 and P2 MeasureDistance must override handleChangedPropertyName:

void MeasureDistance::handleChangedPropertyName(Base::XMLReader &reader,
                                                const char * TypeName,
                                                const char *PropName)
{
    if (strcmp(PropName, "P1") == 0 && strcmp(TypeName, "App::PropertyVector") == 0) {
        Position1.Restore(reader);
    }
    else if (strcmp(PropName, "P2") == 0 && strcmp(TypeName, "App::PropertyVector") == 0) {
        Position2.Restore(reader);
    }
}

Alrigth, i've added the snippets in a new commit and it seems to work well. For some reason the newly created measurements are not visible tho.

@wwmayer
Copy link
Contributor

wwmayer commented May 15, 2024

For some reason the newly created measurements are not visible tho.

I have observed this too but couldn't spot the code block where the view provider fails to show the label. However, if you set the Element links, save the project and reload it the label is shown.

But anyway, it at least shows the distance in the tree view and this is still better than nothing.

@hlorus
Copy link
Contributor Author

hlorus commented May 16, 2024

I found the reason why it wasn't displaying and added a commit.

@maxwxyz
Copy link
Collaborator

maxwxyz commented May 16, 2024

@hlorus maybe off-topic but in the context menu in the 3D view there is still the entries from the old tools. These entries could be removed entirely I don't think a command for the new tool should go there:
grafik

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Measurement: Support of individual picking points for distance
6 participants