Modifying group in api results in duplicate group

[AGGilliam]

I recently came across an idea for creating a class to handle group editing in the api and tried making my own. The code succeeds in modifying the group and perpetuating the changes to existing groups, but the original group ends up being duplicated. Am I correct in assuming that Doc.Create.NewGroup would group the list of elements without placing an additional group instance?

Here's the class I set up:

public class GroupEditor
        {
            Document _doc;
            Group _group;
            GroupType _groupType;
            List<ElementId> _members;
            double _orientation;

            public GroupEditor(Group group)
            {
                _group = group;
                _groupType = _group.GroupType;
                _doc = _group.Document;
                _orientation = _groupType.LookupParameter("Original_Orientation").AsDouble();
            }
            public List<ElementId> Members { get => _members; }
            public void StartEditing()
            {
                _members = _group.UngroupMembers().ToList();
                _doc.Regenerate();
            }
            public void AddElements(List<ElementId> list)
            {
                _members.AddRange(list);
            }
            public void AddElements(ElementId id)
            {
                _members.Add(id);
            }
            public void RemoveElements(List<ElementId> list)
            {
                _members.RemoveAll(x => list.Contains(x));
            }
            public void RemoveElements(ElementId id)
            {
                _members.Remove(id);
            }
            public void FinishEditing()
            {
                _doc.Regenerate();
                _group = _doc.Create.NewGroup(_members);
                _doc.Regenerate();
                FilteredElementCollector collector = new FilteredElementCollector(_doc);
                List<Group> groups = collector.OfClass(typeof(Group)).Cast<Group>().Where(x => x.GroupType.Name == _groupType.Name && x.Id != _group.Id).ToList();
                foreach (Group g in groups)
                    g.GroupType = _group.GroupType;
                string name = _groupType.Name;
                _doc.Delete(_groupType.Id);
                _group.GroupType.Name = name;
                _group.GroupType.LookupParameter("Original_Orientation").Set(_orientation);
            }
        }

I tried adding doc.Regenerate in a few places to see if that would help but that wasn't the issue. If y'all have any thoughts, I'm all ears. Thanks!

[jeremy_tammik]

Just a teeny weeny bit too tricky for me to debug in my head.

I would cleaning up and clarifying this a little bit, if that is possible, by not saving any references to Revit elements or element types while processing.

Your class references the _group and _groupType and extracts some data from them even after you have created the new group, e.g., by referencing _groupType.Name.

Can't you extract all the data ever required up front?

Then it might be possible to minimise and simplify the code snippet creating the new group and deleting the old, to ensure that one and only one group type and the desired number of instances exist.

[RPTHOMAS108]

As Jeremy suggests the code could be cleaner if you consider only the items you need such as ids of group members, id of group type, parameter info you want. In truth you don't event need parameter info since you can get this from existing type just before deleting it (as you do with Name).

Actually I see you reassigned _group in FinishEditing, so that makes sense.

[RPTHOMAS108 · Accepted solution]

From RevitAPI.chm Group.UngroupMembers Remarks:

Removes all the members from the group and deletes the group. Note that the reference to this group object will become invalid once this method is called.

So original group should be deleted if this is called but your class doesn't enforce a system where FinishEditing needs to be called after StartEditing so there is a possible state where this method isn't called, however _members would be empty in such a state but you report groups are edited ok apart from the duplication of original. Need to see procedural code; order of execution. 

A possible cause that comes to mind is that you are reusing the variable '_group'. i.e. this is done all within one transaction with just one call to commit perhaps. So how does the transaction deal with a situation where a variable calls for an API action but is then reassigned before commit is called? Variable has two lifecycles in one transaction only the last being of consequence at time of commit perhaps. I would remove that ambiguity, probably link with unmanaged object (original group) is being broken before commit.

[AGGilliam]

@jeremy_tammik I didn't realize referencing the objects directly like that might cause issues, I tend to leave things a bit open ended so it's easier to debug (at least until I know it works). I swapped my parameters from Group and GroupType to ElementId to try and clean it up a bit but that didn't make a difference.

@RPTHOMAS108 I took your suggestion to swap out the Group reference so I'm not re-using the same variable. Like you said, it was in a single transaction, and I also made sure to StartEditing at the start of the transaction and FinishEditing at the end. The group is still being duplicated though.

I wasn't really sure how else to go about cleaning up this helper class, so I moved the necessary code into the class it's being referenced in but I'm seeing the same issue. All I'm trying to do is modify a single parameter of an element within the group:

using(Transaction t = new Transaction(doc, "stuff"))
            {
                t.Start();

                Reference sel = uidoc.Selection.PickObject(ObjectType.Element);
                Group ogrp = doc.GetElement(sel) as Group;
                //Library.GroupEditor editor = new Library.GroupEditor(grp);
                //editor.StartEditing();
                ElementId _oGroupTypeId = ogrp.GroupType.Id;
                string _oGroupTypeName = ogrp.GroupType.Name;
                double _orientation = ogrp.GroupType.LookupParameter("Original_Orientation").AsDouble();
                List<ElementId> members = ogrp.UngroupMembers().ToList();

                // Update parameters                
                foreach (ElementId id in members)
                {
                    Element e = doc.GetElement(id);
                    if (e is FamilyInstance)
                    {
                        FamilyInstance fi = e as FamilyInstance;
                        if (fi.Symbol.FamilyName.Contains("Panel Wall"))
                        {
                            string height = Library.SelectionTaskDialog(new List<string>() { "8 0", "8 6", "9 0" }, "Heights", "Select a height");
                            fi.LookupParameter("Height").Set(Library.NoUnitStringToFeet(height));
                        }
                    }
                }
                //editor.FinishEditing();

                Group grp = doc.Create.NewGroup(members);
                doc.Regenerate();
                FilteredElementCollector collector = new FilteredElementCollector(doc);
                List<Group> groups = collector.OfClass(typeof(Group)).Cast<Group>().Where(x => x.GroupType.Name == _oGroupTypeName && x.Id != grp.Id).ToList();
                foreach (Group g in groups)
                    g.GroupType = grp.GroupType;
                doc.Delete(_oGroupTypeId);
                grp.GroupType.Name = _oGroupTypeName;
                grp.GroupType.LookupParameter("Original_Orientation").Set(_orientation);

                t.Commit();
            }

[AGGilliam · Accepted solution]

I had another thought after reading your comment to try breaking it up into separate transactions, lo and behold, that solved the issue! Now my question is why? I thought regenerating the document would have been enough, apparently I need to brush up on the transactions documentation. If y'all have any additional thoughts, I'd love to hear them. Thanks again!

[stefanome · Accepted solution]

Here is my solution: https://github.com/stenci/RevitGroupEditor

I have not tried your solution, but I had a similar problem. Look at the comment at line 103 inside FinishEditing, about "Group.UngroupMembers and GroupType.Groups in the same transaction causes GroupType.Groups to return the ungrouped group".

[AGGilliam]

Thanks for sharing! That was actually the solution I linked in my original post. It was a bit more complicated than what I needed but I loved the concept, so I tried to simplify it a bit.

[stefanome]

Oops... I was lazy and didn't read it.

Please look at the comment inside FinishEditing, about "Group.UngroupMembers and GroupType.Groups in the same transaction causes GroupType.Groups to return the ungrouped group".

I haven't looked at your code, but that was causing my groups (only sometimes) to be duplicated.

[AGGilliam]

That's exactly what was happening. I didn't fully understand what you meant the first time I read your comment (would have saved me a headache if I did), but I ended up committing the transaction after ungrouping and that resolved the issue. Also, I didn't realize GroupType.Groups was a thing, that'll help simplify my code a bit as well.

[stefanome]

If the problem is the one described here, then even creating a new variable and scanning again for the group will not solve the problem.

I remember that the problem was difficult to catch because in some cases, perhaps depending on the order of the calls, it wasn't reproducing.

Regenerating didn't help, and creating a transaction would introduce constraints that I didn't like. So I decided to keep track of the group id, and that solved the problem.

[jeremy_tammik]

Another example of the need to regenerate, and sometime more than just that:

https://thebuildingcoder.typepad.com/blog/about-the-author.html#5.33

[RPTHOMAS108]

How this works really depends on what goes on via the unmanaged side. I see it as every API object has it's unmanaged counterpart. So calling functions on a variable that is reassigned before commit just seems wrong (how is unmanaged object keeping track). We've see similar for other things such as setting parameters twice in same transaction. I think it was noted the second change is ignored but I'd have to look back on that.

It shouldn't take two transactions to ungroup a group, takes a dedicated transaction perhaps. It is only ungrouped upon commit (you can do regenerate but has it been ungrouped at that stage?).  Sometime people delete and roll back to identify relations via deleted element ids. So perhaps there is this non-committed change state but not for ungrouping it seems.

In my mind the ungrouping call is not required since the group instance could have been deleted at end and you can read members of it via. Group.GetMemberIds (a passive rather than active function). Perhaps I'm thinking too simplistically on that, since you don't want to create nesting. If it were just about member addition you would be tempted to add a group along with additional objects to a new group and then ungroup it.

Nesting is the real issue for groups.

[stefanome]

I am calling the Ungroup because I want the user to be free to play with the entities inside the group as if they were not grouped. The user clicks on StartEditing, the group is ungrouped, the user deletes some entities, modifies some, creates some and clicks on AddToGroupBeingEdited, then clicks on FinishEditing to regroup the group, that is to create a new group with the updated list of members.

In this workflow StartEditing and FinishEditing happen in two different transactions, so the problem does not exist, but if an add-in wants to call them in the same transaction, that's where things can go wrong.

Since you are sharing what's on your mind, here is what's on my mind: I think that Revit's objects cache tons of stuff for performance reasons, and the behavior we see is just a missing cache invalidation.

Perhaps when we try to get a new Group or GroupType, Revit accesses to some cached data structure that is still holding the stale information and the ungrouped group is still there, and perhaps the only way to invalidate that cache is to commit a transaction.

[AGGilliam]

My intention with ungrouping and regrouping was to modify elements within the group then push those changes to the other instances of that grouptype. It doesn't take two transactions to ungroup the group, there's one transaction to ungroup it and another to modify and regroup it. (I originally had it all in one transaction, but that's where I saw the duplication issue).

I'm a bit confused on your suggestion, wouldn't deleting the group also delete the members? I'm also not sure what you mean by nesting.

[stefanome]

You don't need two transactions.

You can do all in one transaction, as long as you pay attention to the fact that GroupType.Groups will return the ungrouped group.

(well... this is true assuming your problem is the same I had)