Material: Material handling enhancements #10690

davesrocketshop · 2023-09-15T04:08:56Z

Rework of the material handling system.

This is a replacement for #10368, which will be closed in favour of this request.

This first part concentrates on a rework of the material cards. Rather than use a fixed list of possible properties, properties can be defined separately in their own files and mixed to provide a complete list of possible properties. Properties can be inherited.

The cards then provide values for the properties. These can also be inherited allowing for small changes in cards as required.

The new property definitions are more extensive than previously. 2 and 3 dimensional arrays of properties can be defined. Values are obtained by calling an API instead of reading from a dictionary.

For compatibility, a Python dictionary of values can be obtained similar to how it was done previously, but this is considered a deprecated API and won't support the newer advanced features.

The editor is completely reworked. It will be able to edit older format material cards, but can only save them in the new format.

For testing during the development phase, a system preference can specify whether the old or new material editors are to be used. This option will be removed before release.

chennes · 2023-09-18T15:45:31Z

@FEA-eng -- can you take a look at the FEM side of this to make sure I'm not missing any major flaws?

sliptonic · 2023-09-18T15:47:10Z

This is obviously a big one. So we need some concentrated testing and code review.
Maintainers will revisit this at the meeting next week (9/25)

FEA-eng · 2023-09-18T16:48:04Z

@FEA-eng -- can you take a look at the FEM side of this to make sure I'm not missing any major flaws?

I have issues with compilation from source code but I'll try to test this somehow.

davesrocketshop · 2023-09-18T16:51:23Z

@FEA-eng -- can you take a look at the FEM side of this to make sure I'm not missing any major flaws?

I have issues with compilation from source code but I'll try to test this somehow.

There is a new dependency on lib-yaml. If you're on Linux, you'll need to install lib-yaml-dev. On windows, you'll need the latest libpack

FEA-eng · 2023-09-18T16:54:25Z

There is a new dependency on lib-yaml. If you're on Linux, you'll need to install lib-yaml-dev. On windows, you'll need the latest libpack

Well, it's a bigger issue in my case. I haven't been able to compile on Windows without crashes when creating any geometry: https://forum.freecad.org/viewtopic.php?t=73143&start=30#p703935

When it comes to Linux, my virtual Ubuntu machine is missing some dependencies and I don't know how to add them: https://forum.freecad.org/viewtopic.php?t=81204

chennes · 2023-09-18T19:39:01Z

cMake/FreeCAD_Helpers/SetupLibYaml.cmake

+macro(SetupYamlCpp)
+# -------------------------------- YamlCpp --------------------------------
+
+    find_package(yaml-cpp REQUIRED)


Marking this as "REQUIRED" should eliminate the need for the test code after it, right?

chennes · 2023-09-18T19:41:06Z

@davesrocketshop The only changes to FEM appear to be clang-format-based -- is that correct?

davesrocketshop · 2023-09-18T20:09:19Z

@davesrocketshop The only changes to FEM appear to be clang-format-based -- is that correct?

Not quite. The material preferences have been removed from FEM. There are now separate preferences under Materials.

This means the removal of the .ui file, it's corresponding .h and .cpp, CMakefiles, and the line of code that loaded it in AppFemGui.cpp.

chennes · 2023-09-18T21:43:05Z

I don't see those changes in the current version of this PR -- can you double-check that you've git added them?

davesrocketshop · 2023-09-18T21:50:17Z

I don't see those changes in the current version of this PR -- can you double-check that you've git added them?

Looking at the changed files, they're in there. They also show correctly in the PR branch.

chennes · 2023-09-18T22:06:51Z

That's very strange, I don't see any file removals from FEM in this PR. I'll try to look closer this evening.

davesrocketshop · 2023-09-18T22:11:20Z

The removals aren't showing in the "Files changed" section, but they're not there. Can't really explain that. The removed files are:

src/Mod/Fem/Gui/DlgSettingsFemMaterial.ui
src/Mod/Fem/Gui/DlgSettingsFemMaterialImp.cpp
src/Mod/Fem/Gui/DlgSettingsFemMaterialImp.h

The changes are showing in CMakeFiles.txt and AppFemGui.cpp

wwmayer · 2023-09-20T12:46:10Z

src/Mod/Fem/Gui/AppFemGui.cpp

@@ -25,35 +25,26 @@
 #include <Base/Console.h>
 #include <Base/PyObjectBase.h>
 #include <Gui/Application.h>
-#include <Gui/WidgetFactory.h>


I don't know who or what has modified this file but as long as Fem is not listed in the .pre-commit-config.yaml file any formatting changes should be avoided.

This happened when saving the file in VSCode, which is configured to apply the clang rules on saving. The modification was the removal of the material preferences panel, in favour of moving it to a Materials preferences page.

wwmayer · 2023-09-20T12:47:50Z

src/Mod/Material/App/Exceptions.h

+    {
+        this->setMessage(msg);
+    }
+    ~Uninitialized() throw() override = default;


throw() is deprecated. Use noexcept instead.

wwmayer · 2023-09-20T12:52:33Z

src/Mod/Material/App/FolderTree.h

+    {}
+    virtual ~FolderTreeNode() = default;
+
+    NodeType getType(void) const


See modernize-redundant-void-arg: In modern C++ code void shouldn't be use any more as argument of a function.

wwmayer · 2023-09-20T12:53:40Z

src/Mod/Material/App/FolderTree.h

+
+private:
+    NodeType _type;
+    std::map<QString, FolderTreeNode<T>*>* _folder;


Is the ownership of these pointers handled? If not consider using a std::shared_ptr.

wwmayer · 2023-09-20T12:54:49Z

src/Mod/Material/App/MaterialConfigLoader.cpp

+MaterialConfigLoader::MaterialConfigLoader()
+{}
+
+bool MaterialConfigLoader::isConfigStyle(const QString& path)


Fix const-correctness

Static function, so already const correct

wwmayer · 2023-09-20T12:55:05Z

src/Mod/Material/App/MaterialConfigLoader.cpp

+    return true;
+}
+
+QString MaterialConfigLoader::getAuthorAndLicense(const QString& path)


Fix const-correctness

Static function, so already const correct

wwmayer · 2023-09-20T13:01:27Z

src/Mod/Material/App/AppMaterial.cpp

+ *                                                                         *
+ *   This file is part of the FreeCAD CAx development system.              *
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *


For new source files we should use the new copyright text:

/*************************************************************************** * Copyright (c) 2023 David Carter <dcarter@david.carter.ca> * * * * This file is part of FreeCAD. * * * * FreeCAD is free software: you can redistribute it and/or modify it * * under the terms of the GNU Lesser General Public License as * * published by the Free Software Foundation, either version 2.1 of the * * License, or (at your option) any later version. * * * * FreeCAD is distributed in the hope that it will be useful, but * * WITHOUT ANY WARRANTY; without even the implied warranty of * * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * * Lesser General Public License for more details. * * * * You should have received a copy of the GNU Lesser General Public * * License along with FreeCAD. If not, see * * <https://www.gnu.org/licenses/>. * * * **************************************************************************/

wwmayer · 2023-09-20T13:05:09Z

src/Mod/Material/App/MaterialManager.h

+
+public:
+    MaterialManager();
+    virtual ~MaterialManager() = default;


Should be ~MaterialManager() override = default;

wwmayer · 2023-09-20T13:08:14Z

src/Mod/Material/App/MaterialValue.h

+#define MATERIAL_MATERIALVALUE_H
+
+#include <QVariant>
+


Lacks of #include <Mod/Material/MaterialGlobal.h>

Copied from other workbenches but I'll definitely fix

wwmayer · 2023-09-20T13:11:59Z

src/Mod/Material/App/Materials.cpp

+
+MaterialProperty::MaterialProperty()
+{
+    _valuePtr = new MaterialValue(MaterialValue::None);


Possible memory leak! _valuePtr is a raw pointer but the destructor doesn't destroy it.
As said above consider using a std::shared_ptr or std::unique_ptr

wwmayer · 2023-09-20T13:17:58Z

src/Mod/Material/App/PreCompiled.h

+
+#include <FCConfig.h>
+
+#include <Mod/Material/MaterialGlobal.h>


This include shouldn't be needed here because every header that requires the export macro should include it. Only this way the clang code model will be able to correctly parse source files.

wwmayer

See comments in the source code.

davesrocketshop · 2023-09-20T14:19:06Z

See comments in the source code.

Good comments. I'll get to work. It should be noted however that many of your comments are for code copied closely from other workbenches, which is why it was done this way. I also appear to have skipped a few C++ "standardization" meetings, so I'm relearning :)

FEA-eng · 2023-09-24T18:33:04Z

For testing during the development phase, a system preference can specify whether the old or new material editors are to be used. This option will be removed before release.

What is that variable ? I built FreeCAD from your branch but I see the old editor. How can I activate the new one ?

davesrocketshop · 2023-09-24T18:36:50Z

Unselect "Use legacy editor" in the Material preferences

FEA-eng · 2023-09-24T18:52:46Z

Unselect "Use legacy editor" in the Material preferences

The problem is that I don't have "Material" in Preferences. Is there something else that I should do to activate it ?

davesrocketshop · 2023-09-24T19:15:24Z

Unselect "Use legacy editor" in the Material preferences

The problem is that I don't have "Material" in Preferences. Is there something else that I should do to activate it ?

This is a problem I haven't figured out. If you load the workbench it shows up, but I haven't figured out how to do that automatically. There's also no option to load the workbench in the workbench preferences, it just doesn't show up.

As a workaround, in the Python console, type 'import MatGui'. You'll then have the preferences available.

davesrocketshop · 2023-09-25T11:28:30Z

Unselect "Use legacy editor" in the Material preferences

The problem is that I don't have "Material" in Preferences. Is there something else that I should do to activate it ?

I have fixed the issue and merged the changes. If you refresh your repo you will see normal workbench behaviour

chennes · 2023-09-25T14:49:17Z

@davesrocketshop can you rebase? You're probably getting conflicts because we just added FEM to our pre-commit formatter.

davesrocketshop · 2023-09-25T14:53:50Z

@davesrocketshop can you rebase? You're probably getting conflicts because we just added FEM to our pre-commit formatter.

I'd want someone to walk me through that as it's not something I've done before. Every thing I read says don't do it unless you know what you're doing and I definitely don't

chennes · 2023-09-25T14:57:26Z

Sorry, I was using "rebase" as shorthand for "using whatever strategy you like, fix the conflicts that have now appeared with the merge" -- I'd personally git pull --rebase upstream master.

Rework of the material handling system. This first part concntrates on a rework of the material cards. Rather than use a fixed list of possible properties, properties can be defined separately in their own files and mixed to provide a complete list of possible properties. Properties can be inherited. The cards then provide values for the properties. These can also be inherited allowing for small changes in cards as required. The new property definitions are more extensive than previously. 2 and 3 dimensional arrays of properties can be defined. Values are obtained by calling an API instead of reading from a dictionary. For compatibility, a Python dictionary of values can be obtained similar to how it was done previously, but this is considered a deprecated API and won't support the newer advanced features. The editor is completely reworked. It will be able to edit older format material cards, but can only save them in the new format. For testing during the development phase, a system preference can specifiy wether the old or new material editors are to be used. This option will be removed before release.

for more information, see https://pre-commit.ci

davesrocketshop · 2023-09-25T19:11:40Z

Sorry, I was using "rebase" as shorthand for "using whatever strategy you like, fix the conflicts that have now appeared with the merge" -- I'd personally git pull --rebase upstream master.

Done

luzpaz · 2023-09-27T19:56:21Z

What's left to do here ?

davesrocketshop · 2023-09-27T19:58:36Z

What's left to do here ?

I have no actionable items I'm aware of for this PR. I'm already working on the follow on.

chennes · 2023-09-27T21:15:00Z

I was waiting for the CI to get cleaned up in the master branch (an earlier PR broke things) -- since this is going to be a big merge I didn't want to muddy the waters 😄 .

luzpaz · 2023-09-28T00:36:24Z

Congrats @davesrocketshop and FreeCAD as a whole. Really exciting milestone!

github-actions bot added WB FEM Related to the FEM Workbench labels Sep 15, 2023

davesrocketshop mentioned this pull request Sep 15, 2023

[Material] Add Material WB #10368

Closed

chennes self-assigned this Sep 15, 2023

chennes added this to the 1.0 milestone Sep 15, 2023

sliptonic requested a review from wwmayer September 18, 2023 15:45

chennes reviewed Sep 18, 2023

View reviewed changes

wwmayer reviewed Sep 20, 2023

View reviewed changes

davesrocketshop force-pushed the material_merge branch 2 times, most recently from 485be95 to bb83faa Compare September 25, 2023 11:27

davesrocketshop force-pushed the material_merge branch from bb83faa to 2f1fe0c Compare September 25, 2023 11:33

davesrocketshop added 2 commits September 25, 2023 11:04

davesrocketshop force-pushed the material_merge branch from 2f1fe0c to 442bca8 Compare September 25, 2023 15:04

[pre-commit.ci] auto fixes from pre-commit.com hooks

70f3a40

for more information, see https://pre-commit.ci

chennes merged commit dcbb74a into FreeCAD:master Sep 27, 2023
7 checks passed

davesrocketshop deleted the material_merge branch November 2, 2023 12:57


		#include <FCConfig.h>

		#include <Mod/Material/MaterialGlobal.h>

Material: Material handling enhancements #10690

Material: Material handling enhancements #10690

Conversation

davesrocketshop commented Sep 15, 2023

chennes commented Sep 18, 2023

sliptonic commented Sep 18, 2023

FEA-eng commented Sep 18, 2023

davesrocketshop commented Sep 18, 2023

FEA-eng commented Sep 18, 2023

Choose a reason for hiding this comment

chennes commented Sep 18, 2023

davesrocketshop commented Sep 18, 2023

chennes commented Sep 18, 2023

davesrocketshop commented Sep 18, 2023

chennes commented Sep 18, 2023

davesrocketshop commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wwmayer Sep 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wwmayer left a comment

Choose a reason for hiding this comment

davesrocketshop commented Sep 20, 2023

FEA-eng commented Sep 24, 2023

davesrocketshop commented Sep 24, 2023

FEA-eng commented Sep 24, 2023

davesrocketshop commented Sep 24, 2023

davesrocketshop commented Sep 25, 2023

chennes commented Sep 25, 2023

davesrocketshop commented Sep 25, 2023

chennes commented Sep 25, 2023

davesrocketshop commented Sep 25, 2023

luzpaz commented Sep 27, 2023

davesrocketshop commented Sep 27, 2023

chennes commented Sep 27, 2023

luzpaz commented Sep 28, 2023

wwmayer Sep 20, 2023 •

edited