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

Sync Branch 20240619 mme #6 support dynamic blocks read evaluation graphs and block visibility parameters #419

Conversation

mme1950
Copy link
Contributor

@mme1950 mme1950 commented Aug 14, 2024

No description provided.

mme1950 and others added 7 commits June 19, 2024 09:22
…port-dynamic-blocks-read-evaluation-graphs-and-block-visibility-parameters
…graphs-and-block-visibility-parameters' of https://github.com/nanoLogika/ACadSharp into 20240619_mme_#6-support-dynamic-blocks-read-evaluation-graphs-and-block-visibility-parameters
…d-evaluation-graphs-and-block-visibility-parameters
@DomCR
Copy link
Owner

DomCR commented Aug 14, 2024

Hi @mme1950,

I'm planning to release the v1 shortly, but before I would like to change the structure of the project, keep an eye on this PR:

If you plan to implement this feature before, let me know and I will hold the changes.

@mme1950
Copy link
Contributor Author

mme1950 commented Oct 23, 2024

Hi @DomCR,
thank you for your hint. We have synchronized our fork, master and this branch.
So it is ready to be merged from our prespective. And would be helpful to add this functionality to the main project.
As I mentioned earlier we can read a BlockVisibilityParameter object that is linked to a (dynamic) block as far as we need it, but without complete understanding; documentation is missing. The implementation is therefore incomplete.
Perhaps we have a chance that someone can provide more information, when this branch is merged.

@mme1950
Copy link
Contributor Author

mme1950 commented Oct 25, 2024

Hi @DomCR,
"... The implementation is therefore incomplete." means It works very well so far, but I do not know how to make it complete.
But I would be happy if you agree to merge this branch now.

@DomCR
Copy link
Owner

DomCR commented Oct 29, 2024

Hi @DomCR, "... The implementation is therefore incomplete." means It works very well so far, but I do not know how to make it complete. But I would be happy if you agree to merge this branch now.

I don't mind to merge the changes, they are quite isolated and are not breaking anything, but you need to update your branch so the README doesn't change and fix the functionality issues like the Clone() method.

@nka1994
Copy link
Contributor

nka1994 commented Oct 30, 2024

Hello @DomCR, I have commited the corrections.

@DomCR
Copy link
Owner

DomCR commented Oct 30, 2024

One last fix in the Clone() method and we are good to go.

README.md Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should not be modified.

/// Represents an evaluation graph containing a list of <see cref="GraphNode"/>
/// objects.
/// </summary>
public class EvaluationGraph : NonGraphicalObject
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the corresponding DxfName and DxfSubClass attributes for this class.

{
EvaluationGraph clone = (EvaluationGraph)base.Clone();

clone.Nodes.Clear();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Careful here, the List instance is shared for the current instance and the clone, you'll need to initialize a new list to avoid clearing both of them.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is still not fixed, the issue is that when you clone this instance the list will be shared between both:

object current.list <= same instance
object clone.list <= same instance

clone.list.clear() => will also clear the current.list instance

the solution will be:

//Create a new instance of the list before adding the items
clone.Nodes = new ();

/// Gets a <see cref="CadObject"/> associated with this <see cref="CadObject"/>.
/// </summary>
[DxfCodeValue(360)]
public CadObject NodeObject { get; internal set; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be more specific about which CadObject are we storing?
The set is internal but it may help to identify which objects can be linked in the node.

I think they are all related to the subclass AcDbEvalExpr which is shared for BLOCKVISIBILITYGRIP, BLOCKGRIPLOCATIONCOMPONENT, BLOCKBASEPOINTPARAMETER... and more I guess.

/// Represents a BLOCKVISIBILITYPARAMETER object, in AutoCAD used to
/// control the visibility state of entities in a dynamic block.
/// </summary>
public class BlockVisibilityParameter : CadObject
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this class should be moved to the ACadSharp.Blocks folder/namespace, this is the first one that defines a dynamic block but looking to a dxf I can see that there are other classes that share the dxf subclasses like AcDbEvalExpr, AcDbBlockElement, AcDbBlockParameter... if we can identify what they do we can start implementing them.

@DomCR DomCR merged commit 3be9c45 into DomCR:master Nov 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants