3djan/merge implicit #394
3djan/merge implicit #394
Conversation
… in Source/API and Include/API, I am just to lazy to copy manually
…ode type is to much effort. Adding dynamic inputs and outputs to the base node type, and introducing an enum with the node types. This also allows the user to choose between modeling the node types as classes or to choose a dynamic approach.
3dJan
requested review from
vijaiaeroastro,
gangatp,
alexanderoster,
martinweismann and
ewaldbrADSK
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #394 +/- ##
===========================================
- Coverage 71.11% 66.08% -5.03%
===========================================
Files 271 395 +124
Lines 30317 44625 +14308
===========================================
+ Hits 21561 29492 +7931
- Misses 8756 15133 +6377 ☔ View full report in Codecov by Sentry. |
outProduction.3mf
Outdated
There was a problem hiding this comment.
That file looks misplaced
| <error name="INCOMPATIBLEPORTTYPES" code="4000" | ||
| description="Link could not be added, the port types are incompatible" /> | ||
| <error name="GRAPHISCYCLIC" code="4001" | ||
| description="The functin graph is cyclic. Only dircected graphs are valid and can be topological sorted." /> |
There was a problem hiding this comment.
Typo in these words: function and directed
| <param name="Value" type="string" pass="in" description="the value of the metadata" /> | ||
| <param name="Type" type="string" pass="in" description="the type of the metadata" /> | ||
| <param name="MustPreserve" type="bool" pass="in" | ||
| description="shuold the metadata be preserved" /> |
| </method> | ||
| </class> | ||
|
|
||
| <enum name="ImplicitNodeType" description="The type of the node"> |
There was a problem hiding this comment.
ImplicitNodeType and other implicit enums starts from value=1 rather than value=0. Is it by purpose?
| <option name="ArcCos" description="Calculates the arccosinus" value="22" /> | ||
| <option name="ArcTan" description="Calculates the arctangent" value="23" /> | ||
| <option name="ArcTan2" description="Calculates the arctangent" value="24" /> | ||
| <option name="Min" description="Calculates the minimum tow values" value="25" /> |
There was a problem hiding this comment.
typo: tow -> two in option Min
| <option name="Log" description="Natural logarithmus" value="32" /> | ||
| <option name="Log2" description="Logarithmus to the base 2" value="33" /> | ||
| <option name="Log10" description="Logarithmus to the base 10" value="34" /> | ||
| <option name="Select" description="If A is less B returns C, else D" value="35" /> |
There was a problem hiding this comment.
Description change: A less than B
| <option name="ArcTan2" description="Calculates the arctangent" value="24" /> | ||
| <option name="Min" description="Calculates the minimum tow values" value="25" /> | ||
| <option name="Max" description="Calculates the maximum of two values" value="26" /> | ||
| <option name="Abs" description="Calcul the absolute value" value="27" /> |
There was a problem hiding this comment.
typo: calcul -> Calculates
| <class name="FractNode" parent="OneInputNode" description="Returns the fractional part"> | ||
| </class> | ||
|
|
||
| <class name="AbsNode" parent="OneInputNode" description="Calcul the absolute value"> |
There was a problem hiding this comment.
typo: Calcul -> calculates
| <param name="UUID" type="string" pass="return" description="the UUID as string of the form 'xxxxxxxx-xxxx-xxxx-xxxxxxxxxxxxxxxx'"/> | ||
| <method name="GetInputM20" description="Retrieves the input for the element 2 0"> | ||
| <param name="M20" type="class" class="ImplicitPort" pass="return" | ||
| description="the input for the m2 element" /> |
| <param name="SliceStacInstance" type="handle" class="SliceStack" pass="return" description="returns the slicestack instance"/> | ||
| <method name="GetInputM13" description="Retrieves the input for the element 1 3"> | ||
| <param name="M13" type="class" class="ImplicitPort" pass="return" | ||
| description="the input for the m3 element" /> |
| <param name="UUID" type="string" pass="in" description="the UUID as string of the form 'xxxxxxxx-xxxx-xxxx-xxxxxxxxxxxxxxxx'"/> | ||
| <method name="GetInputM21" description="Retrieves the input for the element 2 1"> | ||
| <param name="M21" type="class" class="ImplicitPort" pass="return" | ||
| decription="the input fr the m21 eement" /> |
| description="the added node" /> | ||
| </method> | ||
|
|
||
| <method name="InverseNode" description="Add a InverseNode"> |
There was a problem hiding this comment.
Instead of InverseNode, it should be AddInverseNode to follow other method naming convention.
There was a problem hiding this comment.
The format of this file has been changed, making it hard to find the actual changes. I have figured out a few typos, which can also be ignored. Please check the method renaming suggestion in ImplicitFunction::InverseNode -> ImplicitFunction::AddInverseNode.
| */ | ||
|
|
||
| public: | ||
| CCrossNode() = delete; |
There was a problem hiding this comment.
the default constructor is only deleted for this class. Should it be deleted for other ones?
There was a problem hiding this comment.
We already have a ctor, so no default ctor should be created anyway as long we do not request it explictily. Should be safe to remove this line.
| */ | ||
|
|
||
| public: | ||
| COneInputNode() = default; |
There was a problem hiding this comment.
Is it supposed to be deleted instead of using default like many others?
| if (iIterator != m_ResourceMap.rend()) { | ||
| currentResourceID = iIterator->first + 1; | ||
| } | ||
| // Retrieve a unique Resource ID |
There was a problem hiding this comment.
My initial implementation for this PR #386 was also quite similar. But @alexanderoster and @gangatp mentioned that this would require creating the set everytime and this can make things slower. This is why i introduced m_MaxResourceId.
There was a problem hiding this comment.
Your implementation is problaby much faster, but the key of m_ResourceMap is the UniqueResourceID, not the ModelResourceID.
There was a problem hiding this comment.
occupiedIDs should become a member of CModel, than it only has to be updated when adding or removing resources
| ) | ||
|
|
||
| file(GLOB_RECURSE SOURCES LIST_DIRECTORIES false CONFIGURE_DEPENDS Source/*.cpp) # Recursively get all .cpp files, the false is to not follow symlinks |
There was a problem hiding this comment.
As discussed, this is fixed in ACT itself.
There was a problem hiding this comment.
As discussed, this is fixed in ACT itself.
There was a problem hiding this comment.
Is this used anywhere ?
|
@3dJan @gangatp @ewaldbrADSK I am approving and merging this PR. Since there has not been any other objections, We will proceed as already discussed and fix the issues in develop branch after merging this PR. |
Build workflow: