Nurbs editing Milestone 2

[GregoryLi0]

Milestone 2: Perform advanced operations on brep geometries using command line, as well as some simple ones. Curve operations:

• curve remove
• curve split
• curve copy Surface operations:
• surface remove
• surface split
• surface copy
• create by tensor product
• create by rotating a curve around an axis by an angle
• global surface interpolation with C2 continuity

[GregoryLi0]

Now ''curve/surface remove'' cmds have some potential threats to brep's editing operations. For example, we have a brep with 5 curves with index 0,1,2,3,4. We remove the curve with id = 2, the ids of all curves after this one are reduced by one, get 0,1,2,3. That means previous curve with id = 3 is now id = 2, id =4 is now id = 3. If we don't change others' id, first of all this curve data will be irregular. And 'brep .. plot' and 'brep .. info' cmds will crash dealing with the deleted curve.

[drossberg]

Hi Gregory, Please, check your error and memory management. For example line 443 in src/libbrep/edit.cpp. Shouldn't surface be freed if Create() fails?

You comment to bu_free() is often "free name cpy". Is this intentionally?

You create a new solid and remove the old one often. Is this really necessary? In addition, you remove the solid by calling libged's kill command, which looks a bit heavy for the simple sake of removing an entry from the database. It may have unwanted side effects too. It my be easier to remove a database entry with db_delete(), it this is really neccessary.

            directory* pDir = db_lookup(gib->gb->gedp->dbip, objectName, LOOKUP_NOISE);

            if (pDir != RT_DIR_NULL) {
                if (db_delete(wdbp->dbip, pDir) == 0)
                    db_dirdelete(wdbp->dbip, pDir);
            }

[GregoryLi0]

I'll check and fix these issues as soon as possible. For the third question, I think using db_delete() is better. I asked if we shall create a new brep and remove the old one. Sean said "whether that matters or is even noticeable by users remains to be seen. by doing the edit on a separate copy, that at least avoids a slew of validation code that'd be needed otherwise if/when an edit fails or cannot be applied."

[drossberg]

I checked your conversation with Sean, and the reason isn't apparent from that either.

My point is that you don't change the solid by removing and recreating the brep primitive. You use the same OpenNURBS handle in both objects. I was surprised that this is possible, and I don't recommend to rely on it.

[GregoryLi0]

Do you mean that I kill the old object and the ON_brep object still exists so that it is possible to create a new object with the ON_brep object? I realized that this is really not normal and we should free the object memory when running kill.

I wrote these commands modeled after brep .. flip __brep_cmdflip(). I will see if we can perform nurbs editing operations directly on the old brep object, or if we should copy the object and clean up the old one while running 'kill'.

[drossberg]

I see.

What happens, if you simply omit the "Delete the old object" and "Make the new one" parts?

[GregoryLi0]

I tried omitting them. Memory changes in brep.cpp can be monitored while the code is running, but are not reflected in the archer interface and further output. So I think the ON_brep object passed to brep.cpp is a copy. I plan to update the previous code with db_delete().

[drossberg]

I don't think so. How does it look like (without the kill), if you zap the display and redraw the brep? BTW, what do you mean by "not reflected in the archer interface and further output"? ... Okay, I see. It's strange.

[drossberg]

I think, I know what's going on: On the one hand, you have the database in version 5 format, saved in the .g file, which is an array of bytes. On the other hand, you have an object representation of the primitives, e.g. with rt_brep_internal. I.e. serialization vs. objects.

With your brep_~ functions, you are changing the objects, but not the serialization. I.e., you need them to write back to the database. This is currently done by removing and recreating them there.

However, removing (kill) them should not be necessary. The creation should overwrite an object with the same name.

[GregoryLi0]

Yeah you are right! It works well after removing 'kill'.

[drossberg]

wdb_export_external() in src/librt/wdb.c contains the code to update the objects: "If name already exists, that object will be updated."

[GregoryLi0]

Now I'm using wdb_export() in brep.cpp, which calls wdb_export_external() finally. And everything works fine now.

[drossberg]

I'll accept this PR, despite

• It will need some extensive testing. E.g., I got a program crash because of corrupted memory while executing brep brep_s surface revolution with heavily usage of brep plot before.
• You should reword most (all?) of the "Make the new one" comments to "Update object in database" or similar.

[GregoryLi0]

Thanks! I'll revise them and submit it next PR.