Vikram Atreya (May 18 2021 at 20:13): Vikram Atreya (May 18 2021 at 20:13):Hey @Sean , I'm delighted that I've been selected for GSoC'21 under BRL-CAD and look forward to start work as soon as possible.
I will setup my blog on the org's website soon.
Please do brief me on anything else I should do
Sean (May 18 2021 at 20:21):Hey @Vikram Atreya ! You beat me to it, but yes! I'm also delighted we were able to select you fro GSoC21 and am looking forward to this amazing undo system you'll get going for us. :)
Sean (May 18 2021 at 20:22):I think you have a good handle on things already. You'll want to have a public space (blog, wiki, etc) where you can document your development progress for others. There is a checklist that you'll want to go through, though I know you've already done 2/3rds of it.
Sean (May 18 2021 at 20:24):You'll also want to convert your proposal into a project plan by posting it somewhere public (e.g., our wiki) where we can refine on objectives and to simply keep a record when the project is long over with.
Vikram Atreya (May 18 2021 at 20:26):Sure, will be doing these tomorrow.
Vikram Atreya (May 18 2021 at 20:39):Just a doubt, so from now on will development take place on github, like opening PRs, etc? or we are still continuing with svn?
Daniel Rossberg (May 19 2021 at 06:09):Development takes place on Github. I recommend to fork the BRL-CAD repository to your own account and use this as your working base and for discussions with the mentors. Depending on the state of your work, parts of your work can be shifted back to the main repository via pull request.
Vikram Atreya (May 19 2021 at 16:49):Sean said:
You'll also want to convert your proposal into a project plan by posting it somewhere public (e.g., our wiki) where we can refine on objectives and to simply keep a record when the project is long over with.
I have created the pages for dev logs and project plan, though the content hasn't been filed yet
Dev logs - https://brlcad.org/wiki/User:Vikram_Atreya/GSoC21/Log
Project plan - https://brlcad.org/wiki/User:Vikram_Atreya/GSoC21/Project
Vikram Atreya (May 19 2021 at 16:52):On the communication schedule, I'm planning on putting updates here every 2-3 days and updating the blog every week.
Vikram Atreya (May 21 2021 at 06:39):Updated the project plan in the above link, please do take a look
Vikram Atreya (May 22 2021 at 20:14):Time taken for following in libgit2 method: (haven't divided by CLOCK_CYCLES)
Intialize repo: 4,524,677
open db and make object: 193,198
commit: 4,446,718
delete: 81
Revert back to prev commit: 4,450,182
Time taken for following in partial :
Open and add: 193,797
Kill and add recovery object: 187,787
Recover: 195,973
These times are for a single hairball
Vikram Atreya (May 22 2021 at 20:14):Just a recap of where I had left work
Vikram Atreya (May 22 2021 at 20:15):So the libgit2 method was taking too much time than expected, when compared to the partial+events method
Vikram Atreya (May 22 2021 at 20:16):To continue from here, I will review the libgit2 code once again for any redundancy or extra code that is getting executed
Sean (May 25 2021 at 02:31):Thanks for the numbers @Vikram Atreya ! I got to assume you're using some sort of microsecond timer? Is that a bu_gettime() tick count?
Sean (May 25 2021 at 02:35):If so, the calculation is simply divide by 1M to get seconds, so I have to imagine that's not ticks per microsecond.. otherwise that'd be about 0.2s to undo with partial and 4.4s with git... and those numbers seem wildly high. I would maybe believe 0.02 and 0.4s though.
Sean (May 25 2021 at 02:37):Ah, I bet that's it. I see one of the timers on Windows is 10k ticks per millisecond, i.e., divide by 10M, which would be 0.02 and 0.4s respectively
Vikram Atreya (May 25 2021 at 08:53):Sean said:
Thanks for the numbers Vikram Atreya ! I got to assume you're using some sort of microsecond timer? Is that a bu_gettime() tick count?
I got those values using the clock() function in C++
Vikram Atreya (May 25 2021 at 08:55):So if we divide the values by 10^6, we get the values in seconds
Vikram Atreya (May 26 2021 at 18:28):So I just tested initializing a git repo normally, using git init and it barely took a sec (0.7 sec)
Vikram Atreya (May 26 2021 at 18:29):So I feel I have some redundant/inefficient code in my 'basic undo using VCS' file
Vikram Atreya (May 26 2021 at 18:30):Havent really analyzed in much depth, so will work on it now
Erik (May 27 2021 at 12:15):gardening :) (paying off tech debt? housekeeping?)
Vikram Atreya (May 31 2021 at 12:07):I have rigorously reviewed my code and have made a few changes, the time still remains the same at 13 sec to initialize the repo.
Vikram Atreya (May 31 2021 at 12:08):Next, I tried initializing a git repo with hairball.g using terminal, and (git init + git add + git commit) took 12.5 sec
Vikram Atreya (May 31 2021 at 12:08):Does this make the libgit2 method unviable?
Vikram Atreya (Jun 02 2021 at 20:52):@Sean Could you guide me on what could be my next step so that we are better informed, for selecting the libgit2 method or the partial method?
Sean (Jun 03 2021 at 03:49):Vikram Atreya said:
Does this make the libgit2 method unviable?
I think it means we don't start with it, yes.
Sean (Jun 03 2021 at 03:57):Go with the partial method and we'll just press forward with optimizing it. Maybe we'll learn some other general way to capture state information later on (or use libgit2 as a fallback method later).
Sean (Jun 03 2021 at 03:58):Can you make your latest test code for the two methods available somewhere? Would like to look it over in more detail.
Vikram Atreya (Jun 03 2021 at 03:59):Sure, will upload it as a continuation in the PR I had created
Vikram Atreya (Jun 03 2021 at 04:00):will do in 10 min
Vikram Atreya (Jun 03 2021 at 04:07):Updated the PR
https://sourceforge.net/p/brlcad/patches/569/
Sean (Jun 03 2021 at 04:37):Cool, yeah, just saw the update, great!
Vikram Atreya (Jun 03 2021 at 21:34):I have parallelly started the implementation of the undo command to mged, first goal is to make it print something
Will try to finish this by tomorrow
Vikram Atreya (Jun 04 2021 at 04:42):Done, typing undo in mged prints the title of the database for now. Should I create a PR for this?
Sean (Jun 04 2021 at 05:03):@Vikram Atreya Yes, please do!
Vikram Atreya (Jun 04 2021 at 05:13):Created the patch -- https://sourceforge.net/p/brlcad/patches/570/
Vikram Atreya (Jun 04 2021 at 05:31):Sean said:
With partial, you're basically storing (partial) state as commands are run so we have whatever information is required to restore the previous state. The most simple form of this is to keep a copy of any objects that are changed or deleted. Since we can undo objects added bquoteply deleting them, we don't need to do anything special other than keep track of them.
Note that there's LOTS of ways to implement this sort of undo. For example, here's a really simple setup where (for example) we decide to store our backups in the same .g file using a simple numeric suffix that denotes what change. For example, db.g might have in it: objA objB objC objA;1 objA;2 objA;3 objB;2 objC;4
The current state is three objects: objA objB objC
The most recent change was some edit to objC. When the change was made to objC, we save the previous state is in objC;4 .. so undoing the command is a simple to replace objC with objC;4
The next undo restores objA to a previous state (via objA;3).
The one after that was a simultaneous change to objA and objB, so we restore both from objA;2 and objB;2
Our last discussion on the partial method
Vikram Atreya (Jun 04 2021 at 05:39):The logic to implementing this could be, we store objX;n as you had suggested, then depending on no of undos asked for, we go back to that n and start moving backward to see the latest version of each object until we have verified the latest version of each object
This solves the problem where:
First we make a change to A
Then again change something in A
Make a change in B
Make a change in C
Make a change in A
If we want go back 2 steps, we first see objB;3 next we traverse back and find objA;2 which is the latest version of A before step 3
We still keep going to back to find C but after we see all changes if we do not find it , we delete it (This deletion could be a problem if we had a fixed number of undos stored so we dont find C which could be present earlier and thus deleted, but can be solved easily)
Vikram Atreya (Jun 04 2021 at 18:59):I want to create a stack that can be accessed and modified when any command is called, then is it a must that the stack be a part of gedp?
If not are there any such variables/data strucs that have the same functionality already in the code, so that I can draw parallels when writing code for undo
Vikram Atreya (Jun 05 2021 at 04:52):Some design points I have in mind:
Vikram Atreya (Jun 05 2021 at 21:04):Continuing on the design decisions ......
-n X flag does multiple undos, where X is no of undos to be done Vikram Atreya (Jun 07 2021 at 04:01):I was going through the code of cmd_ged_plain_wrapper in cmd.c, I could not understand what GEDP is, it wasnt even there as a macro in the Doxygen documentation
Vikram Atreya (Jun 07 2021 at 04:13):Its the gedp of the database that is open, right? but still dint get how it is defined
Vikram Atreya (Jun 07 2021 at 05:40):Vikram Atreya said:
I want to create a stack that can be accessed and modified when any command is called, then is it a must that the stack be a part of gedp?
If not are there any such variables/data strucs that have the same functionality already in the code, so that I can draw parallels when writing code for undo
@Sean any help on this, even an int should be sufficient so that I can store at how many steps we have backed up objects, which will be used to create names for the next backup
Erik (Jun 08 2021 at 13:36):GEDP is a struct ged, src/mged/mged[ch]
Vikram Atreya (Jun 08 2021 at 17:44):Updated log and ideas for implementation in project plan
Sean (Jun 09 2021 at 03:14):@Vikram Atreya do you mean some field in struct ged that you can use for tracking version information??
Sean (Jun 09 2021 at 03:22):There is no such field in the struct (nor should their be). Taking a step back, I see you have lots of great thought put into this and lots of questions.
Sean (Jun 09 2021 at 03:25):I think you're biting off a little too much all at once. You got a plan but it's fully of lots of risk and unknowns for you. You don't have any experience working with objects or gedp's yet. This is a hard project, so let's keep this as simple as absolutely possible and build up an undo capability a little bit at a time.
Vikram Atreya (Jun 09 2021 at 03:28):Sean said:
Vikram Atreya do you mean some field in struct ged that you can use for tracking version information??
I wanted to have a tracking variable but did not know where to initialize it.
Yeah I know gedp doesn't have one right now, so was asking if changes in gedp would be required for that or there was some other way
Sean (Jun 09 2021 at 03:30):For example, put a pause on worrying about undoing N times. Pause the thoughts on redo.
Sean (Jun 09 2021 at 03:31):Heck, even forget about any possible command. Let's focus on undoing 1 thing.
Sean (Jun 09 2021 at 03:34):There are nominally three choices then for where you can start. On an object database like our .g file, everything boils down to being one of three things: adding an object, removing an object, changing an object.
Sean (Jun 09 2021 at 03:34):Pick one of those three.
Vikram Atreya (Jun 09 2021 at 03:35):So the task would be just to add an object and then undo it?
Sean (Jun 09 2021 at 03:36):if you picked "adding an object", then yes
Sean (Jun 09 2021 at 03:36):is that what you pick?
Vikram Atreya (Jun 09 2021 at 03:36):Are you talking in terms of adding any object and then undoing it or a hardcoded example?
Vikram Atreya (Jun 09 2021 at 03:37):adding an object using any command, I meant
Sean (Jun 09 2021 at 03:38):I'm talking about thinking of this problem is incredibly oversimplified terms so you will be able to focus on as little code complexity as possible. That focus can initially be on any of those three basic possibilities.
Sean (Jun 09 2021 at 03:38):So, adding an object. Again, simplify, so we're not going to worry JUST yet on any command. Let's focus on just one command that adds an object.
Vikram Atreya (Jun 09 2021 at 03:39):I feel this has already been covered in the patch I had created for hardcoded undo example.
Vikram Atreya (Jun 09 2021 at 03:39):I was exactly doing this
Sean (Jun 09 2021 at 03:39):Sort of, right
Vikram Atreya (Jun 09 2021 at 03:40):Adding an object , making a backup , killing it and then readding it back
Sean (Jun 09 2021 at 03:40):this is about the machinery needed to do it for real in mged
Sean (Jun 09 2021 at 03:40):so conceptually, you know the librt structures involved
Vikram Atreya (Jun 09 2021 at 03:40):Wont be using the ged_make() here as well to add the object?
Sean (Jun 09 2021 at 03:41):Directly, no -- indirectly, yes.
Sean (Jun 09 2021 at 03:41):In your example, the demo had complete knowledge.
Sean (Jun 09 2021 at 03:42):In mged, where's that knowledge going to live?
Sean (Jun 09 2021 at 03:42):does it belong in the .g, in the ged struct, in a librt structure, in an mged structure? what are the tradeoffs?
Sean (Jun 09 2021 at 03:42):they're all options.
Vikram Atreya (Jun 09 2021 at 03:43):Sean said:
In mged, where's that knowledge going to live?
This was exactly my ques regarding the tracking variable :)
Sean (Jun 09 2021 at 03:44):so what I meant about indirect and direct, the 'make' command will be issued on the mged command line. that of course will indirectly and eventually invoke ged_make()
Sean (Jun 09 2021 at 03:45):so the immediate tasks becomes implementing an "undo" command in libed, so it's available as an mged command, that undoes a 'make' command.
Sean (Jun 09 2021 at 03:46):so you can literally perform this sequence, right?
mged> make sph sph
mged> ls sph
mged> undo
mged> ls sph
--> sph not found
Vikram Atreya (Jun 09 2021 at 03:47):Yeah
Sean (Jun 09 2021 at 03:48):So that simple sequence begs a lot of potential complexity right away, but it's important to keep this simple view in perspective for now before jumping to a conclusion.
Vikram Atreya (Jun 09 2021 at 03:50):Sean said:
does it belong in the .g, in the ged struct, in a librt structure, in an mged structure? what are the tradeoffs?
Figuring this out will be an intermediate task right?
Is there any such data, maybe used in another command, stored in any of these so I have some idea on how to implement it for undo
Sean (Jun 09 2021 at 03:50):That's because there are loads of implications that are really complicated to explain, many that we won't even begin to remember until you've coded down a particular path and it becomes really hard to change it, or complexity gets added prematurely. For example, adding a field to ged struct... that is already making an assumption that only struct ged-using applications will be able to undo actions (not an entirely unreasonable assumption, but has implications).
Sean (Jun 09 2021 at 03:52):There really isn't a good pattern for you to follow here in our code base. There are dozens of related patterns on how to pass data around and I'd argue most of them create strong interface couplings and awareness that won't scale well for the needs of an undo system.
Sean (Jun 09 2021 at 03:53):Consider this, archer has an undo system implemented (it was disabled 10+ years ago, but it exists/existed)... it existed before libged existed.
Sean (Jun 09 2021 at 03:54):So if all you had to do was implement undo of make commands, what kind of simple solutions come to mind?
Vikram Atreya (Jun 09 2021 at 03:55):If I could store a state previous to the make, I will know what new elements got added after the make and then kill it
Vikram Atreya (Jun 09 2021 at 03:56):or atleast names of all objects in a previous state
Sean (Jun 09 2021 at 03:56):Answer should be "it depends" ;)
Sean (Jun 09 2021 at 03:57):For example, there's a huge difference between running undo right after running make and running make, restarting mged, and then running undo.
Vikram Atreya (Jun 09 2021 at 03:57):Are we not limiting ourselves to undo in a particular mged session?
Sean (Jun 09 2021 at 03:57):basically, whether that's a requirement determines whether the undo state has to be serialized to disk or only exists for the life of the app
Vikram Atreya (Jun 09 2021 at 03:58):Yes
Sean (Jun 09 2021 at 03:58):Vikram Atreya said:
Are we not limiting ourselves to undo in a particular mged session?
So exactly -- that's a great question and its answer begs for different solution directions.
Sean (Jun 09 2021 at 03:59):I don't recall having decided that one way or the other, but feel free to correct me on that. I think either is a reasonable assumption for starters.
Vikram Atreya (Jun 09 2021 at 04:00):We discussed limiting to a particular mged session when we thought we would go ahead with the libgit2 approach
Sean (Jun 09 2021 at 04:01):Which is funny if true, because the libgit2 approach is intrinsically serialized to disk, which is a means for preserving across processes/sessions
Sean (Jun 09 2021 at 04:02):But sure, let's limit it to a single memory session. So in a given mged session, you get to rely on memory OR disk to store state information. Memory is faster, thus it's reasonable to start there.
Sean (Jun 09 2021 at 04:04):So that brings us back to somehow recording that a database addition has happened in some way that an undo command can be made aware of it.
Sean (Jun 09 2021 at 04:07):Knowing our API, if that were all undo ever had to do, I would probably leverage disk serialization to get cross-process undo for free just because the code to do so would be so incredibly uninvasive. The simplest maintainable solution that comes to my mind would be to have the libged wrapper or parent exec function stash the name of the last 'make' object in the _GLOBAL object.
Sean (Jun 09 2021 at 04:07):That way, make doesn't have to be modified, and undo command is not coupled to the make command in any way. It's coupled to the libged executor.
Sean (Jun 09 2021 at 04:09):the _GLOBAL object is a simple set of key=value pairs, so it'd be very economical to just write a "last_make=object" attribute in there, then undo would read it, delete object, and delete the last_make attribute.
Vikram Atreya (Jun 09 2021 at 04:09):But isnt it better to modify the make command itself? that way even if ged_make were called some other place it would still store the name of the added object
P.S. Is ged_make() exactly equal to make command from mged?
Sean (Jun 09 2021 at 04:09):make sense?
Vikram Atreya (Jun 09 2021 at 04:10):Sean said:
make sense?
the method does, but why write the code in a wrapper or parent exec func?
Why not have a new function in make.c and call it within ged_make_core()
Sean (Jun 09 2021 at 04:16):Vikram Atreya said:
But isnt it better to modify the make command itself? that way even if ged_make were called some other place it would still store the name of the added object
Maybe yes, maybe no. That's my caution about unintended side effects.
Say there's some other "deep_copy" command you don't yet know about that traverses the database N times and calls ged_make()+ged_rename() repeatedly as part of its implementation. If the undo is coupled to some logic in ged_make() then maybe that make "deep_copy" completely unusable (too slow). Also, "undo" very unhelpfully just undoes the last make or perhaps it even fails (crashes?) because in this case every make was followed with a rename.
Sean (Jun 09 2021 at 04:18):P.S. Is ged_make() exactly equal to make from command from ged?
In most cases, yes -- the commands are presently using a convention of ged_[CMD] where CMD is an mged and/or archer command. There are a handful of commands that don't fit the pattern or that have additional logic provided by a common wrapper layer, but I see those abberations as things to fix.
Vikram Atreya (Jun 09 2021 at 04:20):but the wrapper is only called from mged cmds right?
Sean (Jun 09 2021 at 04:20):So while I would use the _GLOBAL, an alternative would be as you suggest -- we could make ged_make() store its action some where. But then we might have to next implement logic like a flag or a switch or a field in a struct to suppress that behavior where it's undesirable.
Sean (Jun 09 2021 at 04:22):Moreover, while we are considering this simple case, we're not blind to the 400 other commands that need to come next. Is editing 400 commands the best we can do for a generalized single-step undo? Probably not... and it probably also implies that we don't want to implement fields or flags that are specific to the make command. Remember there are a couple dozen commands that "add objects"
Sean (Jun 09 2021 at 04:23):Vikram Atreya said:
but the wrapper is only called from mged cmds right?
I'm not sure I understand this question... but no, the wrapper is not called from mged cmds.
Sean (Jun 09 2021 at 04:24):the wrapper is called by mged's libged executor, which calls the given ged_[CMD]() that was registered with it
Vikram Atreya (Jun 09 2021 at 04:25):My ques is like this:
If I type make obj1 obj2 in mged:
calls are: wrapper -> ged_make
but if i were directly calling ged_make() within the code of some other cmd lets say
Then the wrapper attached to make is skipped?
Sean (Jun 09 2021 at 04:26):it entirely depends on the cmd, but yeah it could skip the wrapper. 90% of commands use a generic wrapper that just calls the ged_CMD
Vikram Atreya (Jun 09 2021 at 04:26):Okay.
Sean (Jun 09 2021 at 04:26):the purpose of the wrappers has little really to do with libged
Sean (Jun 09 2021 at 04:28):some commands have different wrappers depending on what they do, like if a command creates geometry (make is a great example), then the wrapper might automatically get the plot data for the object(s) made and draw them into the graphics window so you actually see the object after making it.
Vikram Atreya (Jun 09 2021 at 04:29):Yes, I had gone through the wrappers of a few functions to understand their logic
Sean (Jun 09 2021 at 04:29):If you ran a different command like 'ls', you don't want a wrapper that wastes time trying to figure out if it needs to draw/undraw objects. the wrapper doesn't need to do anything, so it just calls ged_list() instead
Sean (Jun 09 2021 at 04:30):So the wrappers in general make for a lousy storage mechanism for undo because they're not part of the .g and not part of librt or libged, so undo would only work in mged. We definitely want undo to work in mged and archer.
Sean (Jun 09 2021 at 04:31):which results in a choice of duplicating wrapper logic for archer (a bad sign), or finding a different/better way.
Sean (Jun 09 2021 at 04:32):the simplest in-memory place to store the 'last_make' object name is probably struct ged, which is probably now catching up to why you were looking there
Sean (Jun 09 2021 at 04:33):except we don't want undo to be coupled to a specific command still, right? that doesn't scale to 400 commands
Vikram Atreya (Jun 09 2021 at 04:33):True
Sean (Jun 09 2021 at 04:33):we could decide to couple it to the three database operations (add, remove, change)
Sean (Jun 09 2021 at 04:33):but that also feels brittle, begs for a lot of machinery because most commands are sequenced combinations of those
Vikram Atreya (Jun 09 2021 at 04:34):I had another idea planned in mind, which would require a full checkpoint at the oldest command and would not require any additional data to be stored other than the backup objects
Vikram Atreya (Jun 09 2021 at 04:35):Sean said:
the simplest in-memory place to store the 'last_make' object name is probably struct ged, which is probably now catching up to why you were looking there
and even here instead of having last_make, if we can store names of all objects present at a particular state, it would make the job much easier
Sean (Jun 09 2021 at 04:36):that's essentially what libgit2 is doing
Sean (Jun 09 2021 at 04:36):and why it takes so long :)
Vikram Atreya (Jun 09 2021 at 04:37):Pretty much yes :sweat_smile:
Sean (Jun 09 2021 at 04:38):so you might be able to come up with a streamlined higher-performing version of that, but we should design with an assumption that .g files can have 10^5 individual objects as that is about the norm for real geometry.
Sean (Jun 09 2021 at 04:39):about the norm as an upper limit that is, 10^4 is typical along with database sizes in the 100M to 10G range
Vikram Atreya (Jun 09 2021 at 04:40):I think this could work as git was storing a full checkpoint in the beginning and could do a large no of undos, if we restrict ourselves to a smallno like 10-15 it should work fast enough
Sean (Jun 09 2021 at 04:40):don't care much about how usable this is on a .g file with 10 objects if it scales so poorly when there are 10000 objects that mged is no longer interactive for example
Sean (Jun 09 2021 at 04:40):In all your research getting started, how far did you get? Did you learn about the command pattern, memento pattern, and/or undo chains?
Vikram Atreya (Jun 09 2021 at 04:42):I did go through the idea of how things were implemented in command and memento pattern but hadnt seen any code
Sean (Jun 09 2021 at 04:42):If you've not read it yet, here's a good read that might take a half hour or hour and it even pertains to geometric edits: http://blog.wolfire.com/2009/02/how-we-implement-undo/
Vikram Atreya (Jun 09 2021 at 04:43):Will go through it for sure
Vikram Atreya (Jun 09 2021 at 04:45):So my immediate task will be to write code in the wrapper to undo make commands storing last_make in _GLOBAl
Sean (Jun 09 2021 at 04:46):Well you certainly can and there would be absolutely nothing wrong with starting there. It is obvious that that storage mechanism doesn't scale to other commands, so how to decouple it?
Vikram Atreya (Jun 09 2021 at 04:48):Vikram Atreya said:
and even here instead of having last_make, if we can store names of all objects present at a particular state, it would make the job much easier
This is one possibility
Sean (Jun 09 2021 at 04:48):True, but does that scale? Probably not...
Sean (Jun 09 2021 at 04:49):It's also obvious that the storage mechanism will undoubtedly need to change, even probably go away entirely as storing strings in _GLOBAL doesn't seem like that scales too, but the limit doesn't exist until we expand scope to considering undoing lots of additions.
Sean (Jun 09 2021 at 04:50):Think about the name I proposed, and perhaps there's a name less coupled to "make"?
Vikram Atreya (Jun 09 2021 at 04:51):Sean said:
Think about the name I proposed, and perhaps there's a name less coupled to "make"?
name?? U meant method?
Vikram Atreya (Jun 09 2021 at 04:52):You are referring to last_make?
Sean (Jun 09 2021 at 04:52):yes
Sean (Jun 09 2021 at 04:52):last_make
Sean (Jun 09 2021 at 04:52):that's a coupling to the make command
Vikram Atreya (Jun 09 2021 at 04:52):Got it
Sean (Jun 09 2021 at 04:52):instead of a "last_make" key=value attribute, decoupled we might instead chose to name it "last_added" ... that makes it not coupled and conceptually could extend to two more keys for removed and changed, right?
Vikram Atreya (Jun 09 2021 at 04:53):Yeah
Vikram Atreya (Jun 09 2021 at 04:54):We might also want to know the latest change among the three later on
Sean (Jun 09 2021 at 04:54):so the only problem is the fact that we're serializing this junk to disk instead of just keeping it in memory, but I suggest you start there anyways since that will result in an actually functioning undo command we can play with and poke on
Sean (Jun 09 2021 at 04:54):and you still have to decide where to inject the attribute
Sean (Jun 09 2021 at 04:54):Vikram Atreya said:
We might also want to know the latest change among the three later on
BINGO!!
Sean (Jun 09 2021 at 04:57):an evolution might be a latest change, or it might be to combine them together so there's not three attributes which implies a list (and if there's a list, then it can be a stack and the latest will be on the top, right?)
Vikram Atreya (Jun 09 2021 at 04:57):Sean said:
and you still have to decide where to inject the attribute
Does _GLOBAL have a type? like arb8,sph?
Sean (Jun 09 2021 at 04:58):yes, all objects have a major and minor type
Vikram Atreya (Jun 09 2021 at 04:58):Sean said:
an evolution might be a latest change, or it might be to combine them together so there's not three attributes which implies a list (and if there's a list, then it can be a stack and the latest will be on the top, right?)
This was exactly the reason i was asking about the stack in my msg about changing gedp
Sean (Jun 09 2021 at 04:58):_GLOBAL is a non-geometric object type
Vikram Atreya (Jun 09 2021 at 04:58):Vikram Atreya said:
I want to create a stack that can be accessed and modified when any command is called, then is it a must that the stack be a part of gedp?
If not are there any such variables/data strucs that have the same functionality already in the code, so that I can draw parallels when writing code for undo
.
Sean (Jun 09 2021 at 04:59):Vikram Atreya said:
This was exactly the reason i was asking about the stack in my msg about changing gedp
Yes, I realize this, but the answer is super complicated and couplings will be VERY easy to introduce if you jump into the deep end.
Sean (Jun 09 2021 at 04:59):I know this feels like baby steps... there's a reason, I promise. ;)
Sean (Jun 09 2021 at 05:01):Not suggesting I know the answer ahead of time either -- we very well may end up needing to modify the ged pointer (that's what the 'p' in gedp stands for), but we might not too
Vikram Atreya (Jun 09 2021 at 05:02):Sure, I will work on the basic changes today and get back to you at EoD (for me, might be start for you)
Sean (Jun 09 2021 at 05:02):since we're going the partial route, there's a lot of competing concerns like getting as much command coverage as possible without doing per-command work.
Sean (Jun 09 2021 at 05:03):when you finish up undo of make, expand your implementation to the inverse -- a command that deletes an object (e.g., kill)
Vikram Atreya (Jun 09 2021 at 05:03):Okay
Sean (Jun 09 2021 at 05:04):then expand to another command that makes objects (e.g., cp) ...
Sean (Jun 09 2021 at 05:04):each time, revisit your couplings to see what work you had to do to accommodate the change. See if there's a way to eliminate that work.
Vikram Atreya (Jun 09 2021 at 05:05):Sure, will do
Sean (Jun 09 2021 at 05:05):but again, incrementally so we have a chain of commits we can look at to see where directions shift
Sean (Jun 09 2021 at 05:05):where a coupling gets introduced.
Sean (Jun 09 2021 at 05:07):and when you feel like you've got that all working, well talk again -- the next command or change type will dictate new requirements.
Sean (Jun 09 2021 at 05:07):I mean we'll talk before then too, but we'll definitely need a status check after you've got examples of adding, removing, and changing working.
Sean (Jun 09 2021 at 05:09):and make sure the "undo" command is robust. undo without having run make.. what happens? undo with object names after it, what does that mean? etc.
Sean (Jun 09 2021 at 05:09):what makes it throw an error
Vikram Atreya (Jun 09 2021 at 05:09):Okay
Sean (Jun 09 2021 at 05:09):what makes it output anything really
Sean (Jun 09 2021 at 05:11):same thing for return codes.
Vikram Atreya (Jun 09 2021 at 05:14):Will surely consider all these cases when I write the code
Sean (Jun 09 2021 at 05:15):for example,
mged> make sph sph
mged> undo
mged > undo
... does that second undo result in anything printed? does it result in a non-zero return code? a zero return code?
undo will need to be somewhat well-defined (i.e., documented) as you go along. you'll eventually want to write up a simple manual page for it, maybe unit tests too. All things you can tackle when you're needing a break from ged structures and gdb debugging. ;)
Vikram Atreya (Jun 09 2021 at 21:22):I have implemented the last_make variable addition in f_make, ged_make's wrapper
Next, when undo is called it is searching for last_make's value in _GLOBAL and then killing that object using ged_kill()
Vikram Atreya (Jun 09 2021 at 21:25):Should I create a new PR or update the last PR or not required?
Sean (Jun 09 2021 at 21:26):Killing the object sounds wildly wrong.. :)
Sean (Jun 09 2021 at 21:26):_GLOBAL holds other information like the title, working units, and potentially other information
Vikram Atreya (Jun 09 2021 at 21:26):Code is still not robust, have a working version as of now
Will make it more robust and clean tomorrow before making a PR
Sean (Jun 09 2021 at 21:27):It's an attribute object, so you just want to wipe out that specific attribute that was added.
Vikram Atreya (Jun 09 2021 at 21:27):Sean said:
Killing the object sounds wildly wrong.. :)
Was using ged_kill(), so typed it in that flow
Vikram Atreya (Jun 09 2021 at 21:27):Sean said:
It's an attribute object, so you just want to wipe out that specific attribute that was added.
Im not killing _GLOBAL :laughing:
Sean (Jun 09 2021 at 21:27):Then how else are you using ged_kill??
Vikram Atreya (Jun 09 2021 at 21:28):Im deleting the object that has the value stored in last_make
Sean (Jun 09 2021 at 21:28):Oooh, gotya
Vikram Atreya (Jun 09 2021 at 21:28):Pseudo code:
ged_kill(gedp, last_make->value)
Sean (Jun 09 2021 at 21:29):sounds good
Sean (Jun 09 2021 at 21:30):will want to make sure that's robust to last_make->value being null or not existing or whatever else that can happen
Vikram Atreya (Jun 09 2021 at 21:31):Sean said:
will want to make sure that's robust to last_make->value being null or not existing or whatever else that can happen
Yes, I have not dealt with this yet. I will look into this tomorrow.
Vikram Atreya (Jun 09 2021 at 21:36):Along with the robustness changes, I'll try to implement the last_kill and latest_change (among make and kill ) tomorrow.
Vikram Atreya (Jun 10 2021 at 05:36): Vikram Atreya (Jun 10 2021 at 15:07):Done implementing above logic along with latest_change
Now we can make or kill object and then undo that change
Vikram Atreya (Jun 10 2021 at 15:09):I have made the code more robust as well, but not entirely. Will try to wrap this up today and then create a PR before EoD
Vikram Atreya (Jun 10 2021 at 20:05):Done
Vikram Atreya (Jun 10 2021 at 20:05):Errors being handling:
Vikram Atreya (Jun 10 2021 at 20:12):Few bugs I can see right now:
last_change and last_kill are removed but last_make is left, I have intentionally not solved this, since it is not ideal to delete previous info when we work on multiple undos.
Vikram Atreya (Jun 10 2021 at 20:40):Updated PR and logs
Vikram Atreya (Jun 10 2021 at 20:41):@Sean next task would be deal with a command that affects a lot of objects?
or implementing a stack like philosophy into GLOBAL itself, where the change is marked by which no of command it has been generated by
and then while undoing we check for the highest number and then take action on that
Sean (Jun 11 2021 at 03:50):@Vikram Atreya outstanding progress there...
so since you moved on to killing does that mean undo of make is robust?
Vikram Atreya (Jun 11 2021 at 03:51):Yes afaik
Sean (Jun 11 2021 at 03:51):part of the point of the oversimplification is to find and fix all the little corner cases and bugs because it's likely only going to get more complicated as requirement is expanded to other commands.
Sean (Jun 11 2021 at 03:52):cool, so what's it do if you make, rename, then undo?
Vikram Atreya (Jun 11 2021 at 03:53):It wont find the object to kill, so assumes its already killed and removes the last_make and latest_change as of now
Sean (Jun 11 2021 at 03:53):silently or does it print a message?
Sean (Jun 11 2021 at 03:54):it's fine either way, just subtle implications with both, so good to know
Vikram Atreya (Jun 11 2021 at 03:54):I hadnt explicitly tested this, I'll test this right now, just a min
Vikram Atreya (Jun 11 2021 at 03:55):It says db_lookup() failed
Sean (Jun 11 2021 at 03:55):Vikram Atreya said:
Sean next task would be deal with a command that affects a lot of objects?
or implementing a stack like philosophy into GLOBAL itself, where the change is marked by which no of command it has been generated by
and then while undoing we check for the highest number and then take action on that
Nope, the next task is to figure out how to refactor the changes you had to make into a command-agnostic form.
Sean (Jun 11 2021 at 03:56):Vikram Atreya said:
It says db_lookup() failed
Okay, that's a lower-level blather. I suggest turning that off as it's misleading to a user
Sean (Jun 11 2021 at 03:56):there's a quiet flag on lookup
Vikram Atreya (Jun 11 2021 at 03:56):Yes, Ill change that
Sean (Jun 11 2021 at 03:57):So next step, I suggest expanding last_make to include objects made with the 'in' command.
Sean (Jun 11 2021 at 03:58):That's the main primitive creation command that can create nearly any entity interactively.
Vikram Atreya (Jun 11 2021 at 03:58):Sean said:
That's the main primitive creation command that can create nearly any entity interactively.
Just looked it up in the docs
Sean (Jun 11 2021 at 03:59):If you just type a few parameters like "in foo sph" it will prompt you for the rest.
Once completed, you can press up-arrow to see what that command looks like as a 1-liner.
Sean (Jun 11 2021 at 04:00):regardless, the trick is to issue an 'in' and then be able to undo the object
Sean (Jun 11 2021 at 04:00):when you get that working, that will be a very important time to reflect on what code changed and how you can then restructure things better so logic isn't duplicated.
Sean (Jun 11 2021 at 04:02):then try doing the same for a more complex command like killall that kills and modifies
Vikram Atreya (Jun 11 2021 at 04:04):I had one more doubt, is it ok to treat folders as normal objects when backing up?
Sean (Jun 11 2021 at 04:05):so for 'make' and 'in' ... look at what files you edited. the logic will likely be nearly or actually identical. think about what could change so you only have the actual undo logic in exactly one file. where could that be, or what needs to change so that it could be in just one place. Is there a place currently where it could go where undo continues to work in archer also? lots to think about, there are lots of options here. this should take you a couple days to explore.
Vikram Atreya (Jun 11 2021 at 04:05):and even when recovering, will there be any issue of not giving the right references
Sean (Jun 11 2021 at 04:05):Vikram Atreya said:
I had one more doubt, is it ok to treat folders as normal objects when backing up?
er, what the heck is a folder?
Vikram Atreya (Jun 11 2021 at 04:07):Sean said:
er, what the heck is a folder?
Maybe its called a directory
Vikram Atreya (Jun 11 2021 at 04:07):Let me send a picture
Sean (Jun 11 2021 at 04:08):folders live on the filesystem and are effectively logical groupings of files on the filesystem, typically organized into a hierarchical structure...
Sean (Jun 11 2021 at 04:08):directory is a generic concept and could mean lots of things. there is a notion of a directory in a .g file, but I'm not sure that's what you're referring to when you say directory...
Vikram Atreya (Jun 11 2021 at 04:08):image.png
So scene.g was what i meant as a folder, whereas head_tip.s is an object
Sean (Jun 11 2021 at 04:09):Okay, great! So despite the misleading icon... :)
Every word you see listed there is an object.
Sean (Jun 11 2021 at 04:10):bishop.r is an object
Sean (Jun 11 2021 at 04:11):bishop.c is a different object
Sean (Jun 11 2021 at 04:11):headtor.s is yet another object... probably a torus primitive
Sean (Jun 11 2021 at 04:13):the others are called "combination objects" as they are objects defined by combining other objects with CSG Boolean operations
Vikram Atreya (Jun 11 2021 at 04:14):Okay
Vikram Atreya (Jun 11 2021 at 04:15):So there is no issue in directly dealing with these .r or .c then
Vikram Atreya (Jun 11 2021 at 04:16):I always thought, even when writing the concat code that i wasn't making it robust for these "folders"
Sean (Jun 11 2021 at 04:18):from the perspective of a .g file, they are all individual objects that can be directly acted upon
Sean (Jun 11 2021 at 04:18):there are certainly implications, otherwise they would not exist
Sean (Jun 11 2021 at 04:19):but they are just objects. it's only because week peek into the objects and find that some objects reference other objects, and when you pay attention to the references, it becomes obvious that the objects form a hierarchy
Sean (Jun 11 2021 at 04:20):I suggest reading a few things before proceeding, and perhaps doing one of the tutorials that introduces the concept -- it's an important one for what you're going to be doing with undo
Sean (Jun 11 2021 at 04:20):maybe start with https://brlcad.org/w/images/5/52/MGED_Quick_Reference_Card.pdf , look at the second page where it says CAD and explains the three main types of geometric objects
Vikram Atreya (Jun 11 2021 at 04:22):Got it
Sean (Jun 11 2021 at 04:23):then I'd suggest either doing a few of the tutorials at https://brlcad.org/wiki/Documentation under #2 "Introduction to MGED" or reading a lot of #3 "Principles of Effective Modeling" (espeically page 14, but read other parts too)
Sean (Jun 11 2021 at 04:24):the tutorials take like 20 minutes each but incrementally introduce concepts.
Sean (Jun 11 2021 at 04:26):But so yeah, then when you get back to code -- see what kill and undo do when you kill something in the hierarchy. make sure you learn the "tops" and "ls" and "l" commands, understand what they do. they have individual manual pages, but use those three to explore your .g instead of using the geometry browser.
Sean (Jun 11 2021 at 04:27):got to run, but that should be a lot of homework for a while ;)
Sean (Jun 11 2021 at 04:27):keep up the awesome work!
Vikram Atreya (Jun 11 2021 at 04:27):Thank you, yes I'll get to work on these tasks
Vikram Atreya (Jun 12 2021 at 03:39):Sean said:
when you get that working, that will be a very important time to reflect on what code changed and how you can then restructure things better so logic isn't duplicated.
I have implemented the logic for the "in" command, it took me a few seconds to implement since all I had to do was a copy-paste from make's wrapper to in's wrapper.
Vikram Atreya (Jun 12 2021 at 04:43):I have gone through quite a few tutorials in the Introduction to MGED doc, will explore further using a database, and then start work on killall
Vikram Atreya (Jun 13 2021 at 17:05):To reverse a killall is it a good idea to store the parent comb object as backup? My guess is no,but I just wanted to ask
Vikram Atreya (Jun 13 2021 at 18:08):Vikram Atreya said:
image.png
So scene.g was what i meant as a folder, whereas head_tip.s is an object
In this file when I kill, not killall, head.c and then open the geometry browser I get:
db_lookup() failed: head.c does not exist four times
Is this expected? I have removed any code that I might have added but this error still persists
Sean (Jun 14 2021 at 03:51):@Vikram Atreya this is almost certainly a consequence of not doing enough tutorials to understand the hierarchical structure and implications... You probably killed head.c but left a lot of references to it in other combinations. So you'll get an error when looking at any combs that reference it.
Sean (Jun 14 2021 at 04:02):killall may have been too much of a leap .. the reason I mentioned that command next is because it's a kill like you already know plus a set of object changes. maybe instead of killall, do a simpler change command first like the 'g' command. that just affects one object and is easy to reverse. it either creates or changes an existing object.
Vikram Atreya (Jun 14 2021 at 04:04):Sean said:
Vikram Atreya this is almost certainly a consequence of not doing enough tutorials to understand the hierarchical structure and implications... You probably killed head.c but left a lot of references to it in other combinations. So you'll get an error when looking at any combs that reference it.
I do understand how regions, combinations work. I was just wondering if that error was generated by me during the backup for undo
Vikram Atreya (Jun 14 2021 at 04:05):Sean said:
killall may have been too much of a leap .. the reason I mentioned that command next is because it's a kill like you already know plus a set of object changes. maybe instead of killall, do a simpler change command first like the 'g' command. that just affects one object and is easy to reverse. it either creates or changes an existing object.
killall is possible I just need to understand how to do it via commands first, So I've been exploring a lot of commands like copymat and clone but I'm a little confused to where the comb is stored and which commands effectively copy that information as well
Sean (Jun 14 2021 at 04:06):Vikram Atreya said:
I have implemented the logic for the "in" command, it took me a few seconds to implement since all I had to do was a copy-paste from make's wrapper to in's wrapper.
Remember what I wrote ... "so for 'make' and 'in' ... look at what files you edited. the logic will likely be nearly or actually identical. think about what could change so you only have the actual undo logic in exactly one file. where could that be, or what needs to change so that it could be in just one place. Is there a place currently where it could go where undo continues to work in archer also? lots to think about, there are lots of options here. this should take you a couple days to explore."
So yeah, it was a simple copy-and-paste, but now there's two copies of that code. Refactor it to just one copy.
Vikram Atreya (Jun 14 2021 at 04:07):Is it a good idea to create a file in libged/undo with all these functions and they could be called in the wrappers
Sean (Jun 14 2021 at 04:08):Reason for that is that it's a simple copy-and-paste for make and in commands, but there are literally hundreds more and we don't want to do that hundreds of times. Plus, if we want / need it to change, there will be too many instances that need to be edited. Also, there are a dozen variations that aren't as simple, but are nearly the same.
Sean (Jun 14 2021 at 04:09):Sure, you could create a new file if you need to. That's not the hard part... the hard part is figuring out how to change where or how it's called so that it's actually in only one place.
Sean (Jun 14 2021 at 04:11):We may still need to modify all commands, but then we definitely want to do that as few times as possible and not coupled to this particular method.
Sean (Jun 14 2021 at 04:12):What I think we'll end up needing is for commands to either 1) emit events as they do work or 2) perform standard actions through ged wrappers (e.g., for all object creations, deletions, and changes).
Vikram Atreya (Jun 14 2021 at 04:17):So the combinations are not stored in rt_db_internal, but as a separate DS in ged right?
Vikram Atreya (Jun 14 2021 at 04:48):So the exact point where Im stuck is recovering the combination tree again. 2 ways I am thinking of are:
Vikram Atreya (Jun 14 2021 at 04:52):But I'm quite unsure how the first method will work
Sean (Jun 14 2021 at 04:53):Vikram Atreya said:
So the combinations are not stored in rt_db_internal, but as a separate DS in ged right?
I'm not understanding this ... combinations are indeed stored in the .g file and have an rt_db_internal representation.
Sean (Jun 14 2021 at 04:55):Vikram Atreya said:
So the exact point where Im stuck is recovering the combination tree again. 2 ways I am thinking of are:
- Store the comb structure (rt_comb_internal) itself and then paste that when recovering
- Store the name of the parent of the object as well as the object and replicate the "i" cmd and add it back into the db
Perhaps it would help to not think of it as a tree. Think of it as sets of objects. Run "ls" and see all the objects in a database. Running a command like "killall" is going to remove 1 of those objects you see in ls and possibly edit a bunch of others. That is all. Similar with the 'g' command. It's either going to create an object that was not in the "ls" listing or it's going to edit an existing object.
Sean (Jun 14 2021 at 04:57):To "recover the tree", you have to undo the edits and/or add objects and/or remove objects. That's pretty true for nearly all commands.
Sean (Jun 14 2021 at 04:58):there are some efficiencies that can be made by introspecing what types of edits, but we can start blindly and assume any edit needs to be backed up.
Vikram Atreya (Jun 14 2021 at 04:59):Sean said:
I'm not understanding this ... combinations are indeed stored in the .g file and have an rt_db_internal representation.
So there are as many rt_comb_internal s as the no of objects in the .g, right?
Sean (Jun 14 2021 at 05:00):usually there are fewer combinations because there are rt_*_internal for other types too like ell, tor, bot, etc
Sean (Jun 14 2021 at 05:01):there are as many rt_comb_internal as there are combination objects .. that is a combination object in its internal form (i.e., its deserialized in-memory form).
Vikram Atreya (Jun 14 2021 at 05:05):Okay, so when I was going through the code I thought every single object had its own rt_comb_internal, even a non combination object
Sean (Jun 14 2021 at 05:27):they all have an rt_db_internal, maybe you have those two confused?
Vikram Atreya (Jun 14 2021 at 10:34):Yeah I think I'm a little confused, I will go through the data structures once again
Vikram Atreya (Jun 15 2021 at 03:37):Now I'm clear. Usual objects which are made using the "make" commands have rt_db_internal and combination objects made using the "c" command have an rt_comb_internal
Vikram Atreya (Jun 15 2021 at 03:48):To store a backup, first I will have to know which combination has the object, then i can store the combination name in which the object is present and then add back that relation using the "i" command when undo is called
Vikram Atreya (Jun 15 2021 at 03:48):If I search for the comb separately in the wrapper it will double the time taken to run this command, as this search will be happenning twice when the killall command is called
Vikram Atreya (Jun 15 2021 at 03:49):Once inside the wrapper and once in the code to delete it
Vikram Atreya (Jun 15 2021 at 03:49):So will it be okay if I modified the code of killall itself instead of its wrapper (which also affect kill and erase)
Vikram Atreya (Jun 15 2021 at 04:46):Successfully implemented undo for killall by modifying code in killrefs.c
Vikram Atreya (Jun 15 2021 at 04:54):I have presently chosen to undo unions only, but will write more code to accommodate -,+ as well
Vikram Atreya (Jun 15 2021 at 14:41):I have realized that to kill and save backups of combinations and .g objects I will have to write separate code
Right now when Im killing an object, I save a copy of it and hide it.
In case of combinations what happens is the copied object still has the same references as the original object so the children objects never come out of a combination and rather get hidden in the backup
Vikram Atreya (Jun 15 2021 at 20:33):I have also implemented the undo for the g command
Vikram Atreya (Jun 15 2021 at 20:37):I tried refactoring common undo code to libged/undo/new_file.cpp but wasn't able to do it. I feel there can be a better place to place the common code which I will work on next.
Vikram Atreya (Jun 15 2021 at 21:16):Updated logs, will refine code tomorrow and update PR
Vikram Atreya (Jun 16 2021 at 17:52):Refactored make related backup code into mged/utility.c
Vikram Atreya (Jun 16 2021 at 18:15):Bugs/TODO right now:
g undo is exactly the same as make, so when undo is called it kills the g even when the last g command could have been called to just change an existing .g object. Vikram Atreya (Jun 16 2021 at 18:17):First issue is easy to solve
Second and third logic I feel will be similar and something new compared to what I have done till now
Vikram Atreya (Jun 16 2021 at 18:36):Vikram Atreya said:
Updated logs, will refine code tomorrow and update PR
Created new patch since it is not a dummy undo now :grinning:
Vikram Atreya (Jun 16 2021 at 18:37):Please review the patch @Sean so that I can correct any wrong practices that I might be following in the code
Vikram Atreya (Jun 17 2021 at 18:45):I have devised a logic to backup .g /.c objects:
When storing a copy of them we change the names of all the nodes that they are pointing to.
For eg, there exists a group.g with obj1.s , obj2.s and obj3.s in it;
When we store the backup presently the nodes remain the same which means we are never deleting the group but just renaming it, which is undesired
So when we backup we store the group as follows:
group.g_uncommon_suffix
with elements as follows --
obj1.s_uncommon_suffix ,
obj2.s_uncommon_suffix and
obj3.s_uncommon_suffix
This fixes the unwanted referencing problem which is present in the direct copy and store method
Vikram Atreya (Jun 17 2021 at 21:11):Implemeted and tested the above logic. So we can now killall any comb object and recover it without any part of the geometry behaving in any undesired manner :tada:
Vikram Atreya (Jun 17 2021 at 21:12):Vikram Atreya said:
Bugs/TODO right now:
- kill throws an error when flags are used, since I took argv[1] to be the name of the file which is not true when flags are specified
- Storing backup for .g objects that are being killed is flawed. Would like to discuss this Sean
- Logic for
gundo is exactly the same as make, so when undo is called it kills the g even when the lastgcommand could have been called to just change an existing .g object.
I will solve 3. a little later, what should I work on next @Sean
Vikram Atreya (Jun 17 2021 at 21:15):any preference or should I randomly pickup a command?
Vikram Atreya (Jun 18 2021 at 21:42):Implemented undo for mv and mvall
Vikram Atreya (Jun 18 2021 at 21:48):Also restructured code in undo.cpp to accommodate more cmds easily
Vikram Atreya (Jun 19 2021 at 20:23):@Sean I have a doubt, So right now undo code goes into the wrapper and for cmds like kill the backup should happen before the command runs, so if the input is wrong (like name of obj is wrong or wrong no args), problems arise. So how do I overcome this problem because bringing the arg checks into the wrapper will eliminate the purpose of wrappers.
Vikram Atreya (Jun 21 2021 at 16:25):Vikram Atreya said:
Bugs/TODO right now:
- kill throws an error when flags are used, since I took argv[1] to be the name of the file which is not true when flags are specified
- Storing backup for .g objects that are being killed is flawed. Would like to discuss this Sean
- Logic for
gundo is exactly the same as make, so when undo is called it kills the g even when the lastgcommand could have been called to just change an existing .g object.
Solved bug 3
Vikram Atreya (Jun 21 2021 at 17:34):Right now I have not implemented undo for killing multiple objects but while going through all commands available in MGED, I came across this command "keep", this can be very useful to store backups.
So the plan to undo multiple objects being killed is:
Make a backup_file.g
Make a combination cmd_no.g and have all objects that are being killed in that step as its children
Im still unclear about how to recover them/ read from that database and then write it into the present one, will work on it
Vikram Atreya (Jun 21 2021 at 19:34):I have also made a spreadsheet with all commands for which undo is done, not done, or not required
Sean (Jun 25 2021 at 04:12):Vikram Atreya said:
I have also made a spreadsheet with all commands for which undo is done, not done, or not required
This is awesome! That's great organization, but I will just note that many of the commands you have marked as not needing undo could have an undo construct (e.g., opendb and all the rt* commands). They do reversible work or produce outputs that can be "undone".
Vikram Atreya (Jun 25 2021 at 04:13):I have updated only till k actually, after that it got really boring :p
Sean (Jun 25 2021 at 04:13):the real take-away though is that there are hundreds of commands ... so that really begs a question of whether we can design a solution that doesn't require implementing undo hundreds of times and what might that look like?
Vikram Atreya (Jun 25 2021 at 04:14):Will fill in the rest today
Vikram Atreya (Jun 25 2021 at 04:14):Sean said:
the real take-away though is that there are hundreds of commands ... so that really begs a question of whether we can design a solution that doesn't require implementing undo hundreds of times and what might that look like?
I think it can be done decently easily if we can get a list of objects that has changed
Sean (Jun 25 2021 at 04:15):or if not feasible to create a universal undo (sans adopting something like libgit) then how can we structure the code so that logic for a command is localized to that command.
Vikram Atreya (Jun 25 2021 at 04:15):I felt for cmds like dbconcat which are modifying objects without input, that is tough
Vikram Atreya (Jun 25 2021 at 04:15):Any cmd that is making, delting, changing objects via a given input list is easy to undo
Sean (Jun 25 2021 at 04:15):Vikram Atreya said:
I think it can be done decently easily if we can get a list of objects that has changed
That's an excellent observation... I think it's possible too, but haven't 100% convinced myself yet.
Vikram Atreya (Jun 25 2021 at 04:15):Vikram Atreya said:
Any cmd that is making, delting, changing objects via a given input list is easy to undo
should be easy to undo*
Vikram Atreya (Jun 25 2021 at 04:16):Once I can get an implementation for any 1 of those, it should be quite simple to do it for the others
Vikram Atreya (Jun 25 2021 at 04:17):Im exploring the option of storing those objects in another database as of now, but havent yet finished that
Sean (Jun 25 2021 at 04:18):I think a minimum required is for commands to declare a sequenced report of objects added, objects removed, and objects changed. Those are akin to event notifications. It would probably make sense for a few common "meta" events but to be explored later.
Vikram Atreya (Jun 25 2021 at 04:21):Yes, that would help
Sean (Jun 25 2021 at 04:21):Vikram Atreya said:
Im exploring the option of storing those objects in another database as of now, but havent yet finished that
That's great and I think something we'll want, but suggest focusing on making the logic not per-command first if you do indeed believe that is achievable. That will likely involve modifications that are ... complicated.
Vikram Atreya (Jun 25 2021 at 04:21):Vikram Atreya said:
Yes, that would help
but doing it for commands like dbconcat or similar might still be tough
Vikram Atreya (Jun 25 2021 at 04:23):Sean said:
That's great and I think something we'll want, but suggest focusing on making the logic not per-command first if you do indeed believe that is achievable. That will likely involve modifications that are ... complicated.
Right now I have general code to deal with any make or change in one object per command. So it can be dealt by adding 3-4 lines to the wrapper of whichever cmd
Vikram Atreya (Jun 25 2021 at 04:23):but the code is not ready for multi-object make/kill/change
Sean (Jun 25 2021 at 04:24):Vikram Atreya said:
Now I'm clear. Usual objects which are made using the "make" commands have rt_db_internal and combination objects made using the "c" command have an rt_comb_internal
So just to revisit this statement... this is and isn't correct. all objects have an rt_db_internal, combs included.
inside the rt_db_internal is an idb_ptr iirc which is the type-specific structure for that object's type, like rt_comb_internal or rt_ell_internal.
Vikram Atreya (Jun 25 2021 at 04:36):So right now what would be the best thing to work on
Sean (Jun 25 2021 at 04:36):Still checking things over, hold on ;)
Vikram Atreya (Jun 25 2021 at 04:36):Should I continue in the 'new database to store backups' direction
Sean (Jun 25 2021 at 04:37):You've been productive, which is great. ;)
Vikram Atreya (Jun 25 2021 at 04:37):I took a break last 2 days though :P
Sean (Jun 25 2021 at 04:43):Vikram Atreya said:
For eg, there exists a
group.gwith obj1.s , obj2.s and obj3.s in it;
When we store the backup presently the nodes remain the same which means we are never deleting the group but just renaming it, which is undesired
This example...
This fixes the unwanted referencing problem which is present in the direct copy and store method
What exactly was unwanted?
Vikram Atreya (Jun 25 2021 at 04:44):So the method to store backups for kill objects was to create a copy, hide the copy and then kill the object
Sean (Jun 25 2021 at 04:45):Vikram Atreya said:
Sean I have a doubt, So right now undo code goes into the wrapper and for cmds like kill the backup should happen before the command runs, so if the input is wrong (like name of obj is wrong or wrong no args), problems arise. So how do I overcome this problem because bringing the arg checks into the wrapper will eliminate the purpose of wrappers.
This is more or less the point of doing commands like g and make together with other commands like kill. They really beg for a solution that is either not command specific or is very command specific (and thus duplicative).
Vikram Atreya (Jun 25 2021 at 04:45):Vikram Atreya said:
So the method to store backups for kill objects was to create a copy, hide the copy and then kill the object
When this was done to a comb object, all its(the copy) children also got hidden since they were referenced as its children
Sean (Jun 25 2021 at 04:45):Er, they did?? What mechanism did you use to hide them?
Vikram Atreya (Jun 25 2021 at 04:46):but they werent supposed to be hidden, rather displayed independently in the geometry tree, outside the comb object
Vikram Atreya (Jun 25 2021 at 04:46):Sean said:
Er, they did?? What mechanism did you use to hide them?
I changed the names of the leaves. Let me give an example
Sean (Jun 25 2021 at 04:46):oh you mean the didn't show up on "tops" or ?
Vikram Atreya (Jun 25 2021 at 04:46):The example should make things clear 1 sec
Sean (Jun 25 2021 at 04:47):say you have comb1 = comb2 u comb3 and comb2 = prim1 and comb3 =prim2 - prim3 ... now kill comb2
Vikram Atreya (Jun 25 2021 at 04:48):Lets say theres a comb.c with obj1.s, obj2.s, obj3.s
Now kill cmd is given
We make a copy of comb.c, comb_copy.c and then hide comb_copy.c and kill comb.c
Now opening the geometry browser shows nothing, everything is hidden even though the objects shouldn't be hidden
Vikram Atreya (Jun 25 2021 at 04:48):You would expect to see
obj1.s
obj2.s
obj3.s
when we open the geom browser
Vikram Atreya (Jun 25 2021 at 04:49):but it shows to be empty, because the hide property applied to comb_copy.c is being applied to the objects as well
Vikram Atreya (Jun 25 2021 at 04:49):since comb_copy has the same children as comb.c
Vikram Atreya (Jun 25 2021 at 04:50):So I changed the names of the children of comb_copy.c to
obj1.s_undo_suffix, obj2.s_undo_suffix....
Sean (Jun 25 2021 at 04:50):okay, I see what you're getting at. to me that's a problem with the command that the geometry browser is using to get it's listing... ;)
Vikram Atreya (Jun 25 2021 at 04:50):and when recovering the backup i removed the sufffixes(plural for suffix whatever that is)
Sean (Jun 25 2021 at 04:54):got it, understood. that's certainly one possible solution, though I would probably opt for some form of opaque encoding as a way around that. it'll be faster and less likely to cause problems with other commands.
otherwise, the fix for the geometry browser is probably to make the tops command have a flag to ignore hidden objects, and make geometry browser use that flag.
Sean (Jun 25 2021 at 04:56):I suggest trying the prior...
Sean (Jun 25 2021 at 04:56):how are you copying the comb?
Vikram Atreya (Jun 25 2021 at 04:57):usin ged_copy
Sean (Jun 25 2021 at 04:58):oof, okay. Hm. So here's a challenge then.
Sean (Jun 25 2021 at 04:59):instead of ged_copy, I suggest learning the lower-level librt api it uses .. and instead of copying the object, serialize it and save the serialization into a binunif object.
Vikram Atreya (Jun 25 2021 at 05:00):Okay.
Sean (Jun 25 2021 at 05:04):let's see if I can explain it from memory correctly... what you'll do is similar logic that ged_copy is using where you first do a db_lookup() on the object, get its external form (which is basically just an array of bytes), and then instead of exporting that ... you'll create a hidden binunif (see mk_binunif) instead.
Sean (Jun 25 2021 at 05:05):you might want to write a simple little test.c to figure out how to do that first
Sean (Jun 25 2021 at 05:05):that will avoid the whole problem of needing to rewrite the comb (which won't scale to large combs), and avoid the issue of it getting detected in the hierarchy as a comb (so no problem for the geometry browser)
Sean (Jun 25 2021 at 05:06):it's just a binary blob, sort of like how git handles commits
Sean (Jun 25 2021 at 05:06):could even compress it later
Sean (Jun 25 2021 at 05:08):That coincidentally will also let you supply additional flags like what type of undo object is that (a remove, an add, a change, etc), so you can infer how to undo it automatically without having to consult a ledger in _GLOBAL
Vikram Atreya (Jun 25 2021 at 05:08):Ill have to go through the code for this
Vikram Atreya (Jun 25 2021 at 05:08):I'll get back once i read some documentation/code
Sean (Jun 25 2021 at 05:09):how did you end up implementing g ?
Vikram Atreya (Jun 25 2021 at 05:09):Also all db5 objects have the attributes data struc, so we could specify data there as well
Vikram Atreya (Jun 25 2021 at 05:10):Sean said:
how did you end up implementing g ?
It was simple kill
Sean (Jun 25 2021 at 05:10):that's what I meant about being able to specify additional flags for what type of undo object it is
Vikram Atreya (Jun 25 2021 at 05:10):Quite similar to the undo for make
Sean (Jun 25 2021 at 05:10):So then that's not correct :)
Vikram Atreya (Jun 25 2021 at 05:10):also i dealt with the case where the g object already exists
Sean (Jun 25 2021 at 05:10):yes?
Vikram Atreya (Jun 25 2021 at 05:11):Yeah
Sean (Jun 25 2021 at 05:11):I mean ... explain please
Vikram Atreya (Jun 25 2021 at 05:11):So if the g already exists, then I just delete the last object added
Vikram Atreya (Jun 25 2021 at 05:11):added to the .g I mean
Vikram Atreya (Jun 25 2021 at 05:12):so multiobject functionality still isnt there
Sean (Jun 25 2021 at 05:12):Interesting, okay. So you really went the custom-command direction..
Vikram Atreya (Jun 25 2021 at 05:12):but it works for single object additions
Vikram Atreya (Jun 25 2021 at 05:13):Sean said:
Interesting, okay. So you really went the custom-command direction..
Not so much
Vikram Atreya (Jun 25 2021 at 05:13):I stored it as a change ot the g
Vikram Atreya (Jun 25 2021 at 05:14):So i stored a copy of the g before the addition and then recovered it
Sean (Jun 25 2021 at 05:14):It's custom in the sense that you're relying on precise knowledge of the 'g' command adding objects to the end of the list.
Vikram Atreya (Jun 25 2021 at 05:14):Vikram Atreya said:
so multiobject functionality still isnt there
My bad, this is there since Im storing prev state and recovering
Sean (Jun 25 2021 at 05:15):which is why you would need additional logic to make it work for multiple objects
or why you'd need additional logic if there was a switch that added to the front of the list, or a position, or replaces an entry, etc
Sean (Jun 25 2021 at 05:16):It's various modes was kind of the point of that command. It's tricky to make it handle both create and change events cleanly that way.
Sean (Jun 25 2021 at 05:16):Do you have ideas on how that could be implemented differently?
Vikram Atreya (Jun 25 2021 at 05:17):Sean said:
It's various modes was kind of the point of that command. It's tricky to make it handle both create and change events cleanly that way.
It has only 2 modes right?
create and add?
Vikram Atreya (Jun 25 2021 at 05:18):Sean said:
which is why you would need additional logic to make it work for multiple objects
or why you'd need additional logic if there was a switch that added to the front of the list, or a position, or replaces an entry, etc
I did not get this entirely
Vikram Atreya (Jun 25 2021 at 05:18):What is the list you are referring to?
Sean (Jun 25 2021 at 05:18):the comb recipe
Sean (Jun 25 2021 at 05:19):comb1 = obj1 u obj2 u obj3 - obj4 u obj5, etc.. it can be thought of as a stack or a list
Vikram Atreya (Jun 25 2021 at 05:19):Okay
Vikram Atreya (Jun 25 2021 at 05:19):Right now the code doesnt depend on the order of the list if you were wodnering
Vikram Atreya (Jun 25 2021 at 05:20):Im storing the whole .g object and then replacing it
Sean (Jun 25 2021 at 05:20):Oh? I thought you just said it doesn't handle N objects getting added?
Vikram Atreya (Jun 25 2021 at 05:21):Vikram Atreya said:
Vikram Atreya said:
so multiobject functionality still isnt there
My bad, this is there since Im storing prev state and recovering
.
Vikram Atreya (Jun 25 2021 at 05:21):I got confused at first, but I checked the code now. Im dealing with it as a change in an object
Vikram Atreya (Jun 25 2021 at 05:21):So entire .g gets replaced
Sean (Jun 25 2021 at 05:22):Ah, excellent then. That's good.
Vikram Atreya (Jun 25 2021 at 05:22):So in a way the code has only been customised for the modes of the g cmd
Vikram Atreya (Jun 25 2021 at 05:22):and not for the specific addition
Sean (Jun 25 2021 at 05:22):excellent, because I think that is the basis of what you need next...
Sean (Jun 25 2021 at 05:23):so what commands do you presently have? make + g + kill ?
Vikram Atreya (Jun 25 2021 at 05:23):So most commands that have a make function like cp, mirror, cpi,arb
Vikram Atreya (Jun 25 2021 at 05:24):I have undo for move
Vikram Atreya (Jun 25 2021 at 05:24):I have general code for a changed object but has been applied only for the g command as of now
Vikram Atreya (Jun 25 2021 at 05:24):And yes kill but only single object
Sean (Jun 25 2021 at 05:25):Vikram Atreya said:
So most commands that have a make function like cp, mirror, cpi,arb
Is that because they call ged_make() ? or did you edit them?
Vikram Atreya (Jun 25 2021 at 05:25):deleted
Vikram Atreya (Jun 25 2021 at 05:26):Sean said:
Is that because they call ged_make() ? or did you edit them?
I edited their wrappers
Sean (Jun 25 2021 at 05:26):did they have a common wrapper or multiple wrappers or?
Vikram Atreya (Jun 25 2021 at 05:26):common wrapper, i added an if statement
Vikram Atreya (Jun 25 2021 at 05:27):coz there was no point creating a new wrapper for just 3 more lines
Sean (Jun 25 2021 at 05:27):okay, not so bad then. will probably need to rework that, but it's at least isolated.
Sean (Jun 25 2021 at 05:28):so the next steps I think are:
Sean (Jun 25 2021 at 05:28):1) figuring out how to use binunif as the storage object
Sean (Jun 25 2021 at 05:29):2) making commands emit events for add/change/remove
Vikram Atreya (Jun 25 2021 at 05:32):Sean said:
2) making commands emit events for add/change/remove
Where do they emit though?
to the attributes of the object?
Vikram Atreya (Jun 25 2021 at 05:33):and also we would want the emit code to be in the wrapper, right?
Sean (Jun 25 2021 at 05:33):figuring out #2 is going to be the more complicated as there are choices, but how about this:
Sean (Jun 25 2021 at 05:40):have three functions ged_undo_add(), ged_undo_remove(), and ged_undo_change() that do the actual work given an object name. then probably modifying GED_DB_PUT_INTERNAL.
Sean (Jun 25 2021 at 05:40):and THEN ... you'll almost certainly have to introduce something for removals
Sean (Jun 25 2021 at 05:41):as there's not a GED_DB_DELETE() but probably should be where you'd record the event
Sean (Jun 25 2021 at 05:42):so for GED_DB_PUT_INTERNAL and GED_DB_DELETE, you just record the change/add/delete event into the struct ged *
Sean (Jun 25 2021 at 05:42):then you handle the entire undo changeset recorded after the command has completed (basically a wrapper around all the wrappers)
Vikram Atreya (Jun 25 2021 at 05:43):I dint get where to record it, as an attribute to _GLOBAL?
Sean (Jun 25 2021 at 05:45):No, I mean you could, but probably not as that's brittle. So yeah, next step 1.5) change from using _GLOBAL to using struct ged and the undo objects.
Sean (Jun 25 2021 at 05:47):welcome to keep it on _GLOBAL for now as that's not a critical step
Vikram Atreya (Jun 25 2021 at 05:47):Right now Im emitting changes for single objects
Vikram Atreya (Jun 25 2021 at 05:47):So doing it for multiple objects is the task now, right?
Sean (Jun 25 2021 at 05:48):Right, so that all would completely change in _GLOBAL anyways, that's part of the reason to figure out a different container in struct ged anyways
Vikram Atreya (Jun 25 2021 at 05:51):But I havent been able to do that in ged
Vikram Atreya (Jun 25 2021 at 05:51):That was the starting point in my GSoC jouney hehe
Sean (Jun 25 2021 at 05:51):yeah, I remember :)
Sean (Jun 25 2021 at 05:52):the hope is that you're now a lot more experienced with the structures and functions to understand some of the implications
Sean (Jun 25 2021 at 05:53):I mean, it doesn't need to be fancy. shouldn't be fancy. It can be a simple linear list of object additions, changes, and removals. something like a linked list of struct ged_change { enum type; const char *obj; } and struct ged would have a struct ged_change *gcp; that is acted upon by GED_DB_PUT_INTERANL, GED_DB_DELETE, and maybe others.
Sean (Jun 25 2021 at 06:00):the big piece missing is a place to inject logic before and after commands are run. this exists -- it's ged_exec() but mged coincidentally does not call that yet. so we might need to sort that out first... so maybe step 3 instead of 1.5...
Vikram Atreya (Jun 25 2021 at 06:01):Sean said:
sort that out first... so maybe step 3
first or third?
starseeker (Jun 25 2021 at 12:26):I don't know if it's helpful for this discussion, but for qged I added a flag in struct directory called "edit_flag" which is set by db_put_external. That's how I'm able to know I need to update the visuals for objects in qged after a ged command is run - I zero out all directory edit_flags, run the command, and then check to see if any of them were tripped. Anything with the flag set needs to get redrawn.
This may not be suitable for the undo mechanism, but at least for my purposes it is simple and effective - I don't have to care about the details of what the logic called by ged_exec did, so I'm completely isolated from any command logic or command changes.
Vikram Atreya (Jun 25 2021 at 14:35):Yes, I do think that helps. I am trying to do something similar but maybe to the ged data struc.
starseeker (Jun 25 2021 at 15:05):The advantage of db_put_external is that anything writing to the .g file is going to go through there.
starseeker (Jun 25 2021 at 15:07):The "do I need to draw" problem for the UI is similar to undo, in that sense. libtclcad and MGED handle it by defining different types of wrapper functions and assigning one to each command, which is less than ideal.
Vikram Atreya (Jun 25 2021 at 17:16):Vikram Atreya said:
Sean I have a doubt, So right now undo code goes into the wrapper and for cmds like kill the backup should happen before the command runs, so if the input is wrong (like name of obj is wrong or wrong no args), problems arise. So how do I overcome this problem because bringing the arg checks into the wrapper will eliminate the purpose of wrappers.
I see you have reacted to this with a smile :joy: , does that mean I got to solve this on my own?
Well I can by bringing the options code into the wrapper but all common wrappers would then get a switch or an if/else to check for the inputs
Vikram Atreya (Jun 25 2021 at 18:43):When creating the binuif what data type should I input float, double, 32bit init, etc?
Vikram Atreya (Jun 25 2021 at 18:44):As far as I understand it doesn't directly affect whichever data type I use, but please let me know if there are some do's/dont's on which type to select
Vikram Atreya (Jun 25 2021 at 22:08):<deleted>
Vikram Atreya (Jun 25 2021 at 22:08):<deleted>
Vikram Atreya (Jun 25 2021 at 22:11):<deleted>
Vikram Atreya (Jun 25 2021 at 22:22):<deleted>
Vikram Atreya (Jun 25 2021 at 22:37):Which kind of objects have major type as BINARY_MASK?
Right now my simple tests stops because the object im trying to convert doesnt have majortype binary_mask
Sean (Jun 26 2021 at 04:53):Vikram Atreya said:
Sean said:
sort that out first... so maybe step 3
first or third?
third. it has to be sorted out before you can utilize ged_exec, but the other parts are more important for geting set up for improving the object backup scheme.
Sean (Jun 26 2021 at 05:26):starseeker said:
I don't know if it's helpful for this discussion, but for qged I added a flag in struct directory called "edit_flag" which is set by db_put_external. That's how I'm able to know I need to update the visuals for objects in qged after a ged command is run - I zero out all directory edit_flags, run the command, and then check to see if any of them were tripped. Anything with the flag set needs to get redrawn.
This may not be suitable for the undo mechanism, but at least for my purposes it is simple and effective - I don't have to care about the details of what the logic called by ged_exec did, so I'm completely isolated from any command logic or command changes.
@starseeker ... really hate to say it, but that is a straight up scope violation for that structure. I mean I get how and why it works and why you thought to put it there, but I do not think it's right from a design perspective. I hadn't seen that commit yet or I would thought it worth discussion.
To frame it by way of an analogy, it's like cleaning staff putting stickers or sharpie circles around stores on the public directory kiosk in mall to help keep track of which stores have been cleaned recently. Sure it works for them, works GREAT for them... but that's in no way what the directory is for and is irrelevant/distracting/confusing to essentially everyone else.
In CS terms, it's a data specialization that doesn't intrinsically fit the structure (from both an OO perspective or a data-driven procedural perspective). It's also couping a new concept (a notion of edit state and/or views) to what was otherwise a pure database construct with no awareness or linkage to views, edit state, interface. Def minor point, but it's also a waste of a couple bytes per object, so it's that much more pressure on memory use, cache misses, context switch overhead.
Now that's not to suggest that another structure is more appropriate, it's specifically the notion of what an edit flag means in a general sense on a directory entry. Is a non-zero value supposed to mean recently edited? What constitutes recently? What does a value of -8 mean? Directory structs were stateless, and that appears to be some sort of live state -- so when does it change? Can my code change it?
You can probably achieve the same effect by using the u_data pointer or by adding a callback so calling code is notified when something happens to the directory entry.
Sean (Jun 26 2021 at 05:36):starseeker said:
The advantage of db_put_external is that anything writing to the .g file is going to go through there.
I think that's too low-level. At least, I do not see undo as an intrinsic feature of the .g, and would not belong in "libg" if it existed (thus doesn't belong in librt currently). It's a higher level application-level construct, thus belongs somewhere in libged or higher.
As we think of undoing commands, that to me says libged is the most appropriate place for the logic to reside and/or be registered. If I run a killtree, I'd expect undo to reverse the entire killtree in one step as a single transaction (which is a concept librt/libg knows nothing about). It might conceptually fit if we introduced transactions to librt/libg, but that's unquestionably in scope for libged today where the killtree command exists, where it's aware of the set of things it's going to change.
Sean (Jun 26 2021 at 05:40):Vikram Atreya said:
I see you have reacted to this with a smile :joy: , does that mean I got to solve this on my own?
Well I can by bringing the options code into the wrapper but all common wrappers would then get a switch or an if/else to check for the inputs
I responded to the doubt/statement a little further down. "This is more or less the point of doing commands like g and make together with other commands like kill. They really beg for a solution that is either not command specific or is very command specific (and thus duplicative)."
You can't bring the options code into the wrapper.
Sean (Jun 26 2021 at 05:49):Vikram Atreya said:
Which kind of objects have major type as BINARY_MASK?
Right now my simple tests stops because the object im trying to convert doesnt have majortype binary_mask
None have BINARY_MASK... that's a bit mask, so it matches DB5_MAJORTYPE_BINARY_UNIF and DB5_MAJORTYPE_BINARY_MIME is all.
Sean (Jun 26 2021 at 05:50):You'll want to create a DB5_MAJORTYPE_BINARY_UNIF with just bytes (or maybe ubytes). You have an array of bytes after you get external. You're just putting that array of bytes into a binunif object (which is a lot like a BLOB object type if you're familiar with those).
Sean (Jun 26 2021 at 05:51):mk_binunif should create the type correctly for you -- you just provide the char* and number of bytes.
Vikram Atreya (Jun 26 2021 at 18:27):I have successfully written a basic test to use mk_binunif() and create a binary object from an geometry object
Vikram Atreya (Jun 26 2021 at 18:56):I have read the code in bo cmd, but that is to write the bin object to a file
Im checking how to recover the object
Vikram Atreya (Jun 26 2021 at 19:59):Is there an already implemented way to convert these binary objects back to database objects?
Vikram Atreya (Jun 27 2021 at 17:51):I got vaccinated today so I'm feeling under the weather and won't be working today
This week progress has been a little slow, so plan is to catch up next week
Rn Im still stuck at converting the binary object back to a drawable object
Sean (Jun 28 2021 at 06:28):@Vikram Atreya Glad to hear that! Not that you're unwell of course, but it's a good form of unwell overall. Hope you get some rest and are better today.
Vikram Atreya (Jun 28 2021 at 06:30):Yeah Im feeling better today :grinning:
Sean (Jun 28 2021 at 06:32):Vikram Atreya said:
Is there an already implemented way to convert these binary objects back to database objects?
I'm not sure we have code, but the gist is to do a lookup, get the rt_db_internal, then cast the idb_ptr (which will be an struct rt_binunif_internal*), then simply access the bip->u.uint8 which will be the bytes you packed.
Vikram Atreya (Jun 28 2021 at 06:57):So just copy the bip->u.uint8 into the new the drawable object's internal
Vikram Atreya (Jun 28 2021 at 23:21):Done!
I have been able to store an object into binary object and recover the object back if we know its type(ell, arb, etc)
Vikram Atreya (Jun 28 2021 at 23:25):When storing the backup of an object, we could store the name and type as attributes of the binary object
Vikram Atreya (Jun 29 2021 at 00:10):Now Im working on changing the code of kill wrapper, so that backup is made via binary objects
Vikram Atreya (Jun 29 2021 at 00:54):Vikram Atreya said:
Now Im working on changing the code of kill wrapper, so that backup is made via binary objects
I have done for combination objects, can be done for all objects as well but the code will be very long mostly due to the memory allocation switch , and as there is no problem right now in storing them as normal objects should/can I leave it like that?
Vikram Atreya (Jun 29 2021 at 01:25):Made changes to g wrapper and undo to store and recover from binary objects respectively
Vikram Atreya (Jun 29 2021 at 01:41):Sean said:
2) making commands emit events for add/change/remove
@Sean could you better explain the scope of this step?
Rn Im already storing latest_change, last_create/last_killed/last_changed attributes in _GLOBAL for cmds (make, g, arb, etc; kill, killall; )
Vikram Atreya (Jun 29 2021 at 01:42):Should I do this for all cmds that are available?
Sean (Jun 29 2021 at 04:29):Vikram Atreya said:
When storing the backup of an object, we could store the name and type as attributes of the binary object
That should happen for you automatically when you get the external... the bytes for the object that you put into the binunif should be the external stream that is serialized to disk. That has the name and type information.
Sean (Jun 29 2021 at 04:29):Check out an example for doing this in src/gtools/gtransfer.c .. the server_geom() function and a couple others do something similar.
Vikram Atreya (Jun 29 2021 at 04:31):Sean said:
That should happen for you automatically when you get the external... the bytes for the object that you put into the binunif should be the external stream that is serialized to disk. That has the name and type information.
Okay. As of now Im storing the rt_db_internal in the binunif. Intially I was storing the external
Sean said:
I'm not sure we have code, but the gist is to do a lookup, get the rt_db_internal, then cast the idb_ptr (which will be an struct rt_binunif_internal*), then simply access the bip->u.uint8 which will be the bytes you packed.
But after I read this I changed everything to internal
Sean (Jun 29 2021 at 04:33):Vikram Atreya said:
Should I do this for all cmds that are available?
Definitely not as it's still fundamentally flawed in terms of undoing commands that result in sequences of changes.
Sean (Jun 29 2021 at 04:38):You can store the internal too, but then like you noted, you'll also have to store more information and that then couples the implementation tightly to the current data structure instead of to API.
Sean (Jun 29 2021 at 04:38):Better is to either convert the internal to an external or do a lookup and get an external from the directory* via db_get_external().
Vikram Atreya (Jun 29 2021 at 04:38):Yes I realize storing external is a better option
Sean (Jun 29 2021 at 04:43):gtransfer uses that method in the client to get an external from db_get_external(), and in the server, it does some fancy processing to stash the object into an in-memory only database so it can raytrace it without touching the disk.
Vikram Atreya (Jun 29 2021 at 20:34):Changed it from storing the internal data to bu_external
Encountered a lot of bugs so took quite some time :sweat:
Vikram Atreya (Jun 29 2021 at 21:18):Sean said:
Definitely not as it's still fundamentally flawed in terms of undoing commands that result in sequences of changes.
Like for cmd killtree? a kill event should be stores for every object it is deleting ?
Vikram Atreya (Jun 29 2021 at 21:20):I am also planning to do code cleaning and testing at the end of this week and create a PR which can robustly handle 1 undo for (make&co-cmds), kill and g
Sean (Jun 29 2021 at 21:41):Yeah a command like killtree. Somewhere somehow they need to be grouped, so the command can be undone, not individual objects.
I'd suggest we think of it simply as a change event which is a sequence of 1 or more add/edit/kill changes to the database.
Sean (Jun 29 2021 at 21:45):I can think of a couple ways to easily achieve that type of storage. You could write out every object like you're doing now as a binunif for example representing the add/edit/kill action, but then list them in an object that is also serialized to a binunif. So when you try to undo that object, you see it's a list of changes, and undo all those subchanges.
Vikram Atreya (Jun 29 2021 at 21:48):but we will always need a list of all the objects that have to be deleted/overwritten right?
Vikram Atreya (Jun 29 2021 at 21:48):because we do not know the names of the objects till we extract the binary object
Sean (Jun 29 2021 at 21:48):or added back
Sean (Jun 29 2021 at 21:49):you actually could put the name on the binary object if you want as an attribute
Sean (Jun 29 2021 at 21:49):that way you don't have to unpack the binunif to figure out what's in there
Vikram Atreya (Jun 29 2021 at 21:49):Sean said:
you actually could put the name on the binary object if you want as an attribute
Yeah Im doing that right now
Sean (Jun 29 2021 at 21:49):or even use some sort of name convention
Vikram Atreya (Jun 29 2021 at 21:50):Sean said:
I can think of a couple ways to easily achieve that type of storage. You could write out every object like you're doing now as a binunif for example representing the add/edit/kill action, but then list them in an object that is also serialized to a binunif. So when you try to undo that object, you see it's a list of changes, and undo all those subchanges.
but for this we will have to store the changes somewhere or the name will be toooo long
Sean (Jun 29 2021 at 21:51):e.g., .1.undo.HASH.my_object might be the 1st undo action, pertaining to my_object
Sean (Jun 29 2021 at 21:52):.392.undo.20194e9678ff7a08df564c3f83a4bd86.my_object
Sean (Jun 29 2021 at 21:52):392 undo action, also some change/rm/add to my_object
Vikram Atreya (Jun 29 2021 at 21:54):Just checking, Here the number is referring to the undo step in the database and not specific to the object
Sean (Jun 29 2021 at 21:56):so what I was saying about combs is a little different. that'd be something like .123.undo.HASH and inside that binunif object is a list of .122.undo.HASH, .121.undo.HASH, .120.undo.HASH, and .117.undo.HASH for example. and you'd recursively unpack the binunifs of 122, 121, 120, and 117 to make sure the entire 123 set can be undone. (say 120 is also a set too of 119 and 118 to undo).
Sean (Jun 29 2021 at 21:56):yeah, it's just the undo sequence
Sean (Jun 29 2021 at 21:56):though I should make clear ... it doesn't have to be like this -- this is just spitballing ideas quickly!
Sean (Jun 29 2021 at 21:57):you just somehow ideally need to look at a set of undo objects and 1) know which was the latest, and 2) know the sequential ordering of all objects
Vikram Atreya (Jun 29 2021 at 21:58):This feels like a good idea, I will parallelly think of other ideas as well
Sean (Jun 29 2021 at 21:58):that way we can always undo or undo -n10 or whatever
Sean (Jun 29 2021 at 21:59):knowing the sequence would also let us cherry pick undo actions, BUT then we'll have to think through how to deal with object renamings
Sean (Jun 29 2021 at 21:59):if they're always sequential, that in theory shouldn't be an issue ... I think
Vikram Atreya (Jun 29 2021 at 21:59):Sean said:
if they're always sequential, that in theory shouldn't be an issue ... I think
yes, if we go back undoing every change it will also account for the renaming
Sean (Jun 29 2021 at 22:03):It still will probably need to check though, because some external program could have modified the database outside of libged
Sean (Jun 29 2021 at 22:03):it just might not be able to undo, or might be only able to partially undo and such.
Vikram Atreya (Jun 29 2021 at 22:05):If it is not too much overload, names of all existing object could be stores at each step and extra objects could be deleted after the undo
Sean (Jun 29 2021 at 22:06):Thats an interesting idea. I'm not sure it'd scale well but maybe... would require testing on real/big databases.
Sean (Jun 29 2021 at 22:16):If you want to make a fake real one to play with, you can run something like the clone command and make a 10 x 10 grid of copies in a scene of some database like m35, and then group them all into a scene. That should be something like 50k objects total of which about 23k are combinations. That's more realistic.
Vikram Atreya (Jun 29 2021 at 22:26):Next I will try working on killtree then
Sean (Jun 30 2021 at 06:35):@Vikram Atreya Here's a copy of the m35 patterned into a grid that is closer to the complexity found in a fully detailed model.
m35s.g.gz
Sean (Jun 30 2021 at 06:35):Screen-Shot-2021-06-30-at-2.31.26-AM.png
Sean (Jun 30 2021 at 06:37):It's about 90k objects, about half of which are combinations. It's still not representative of the file size and memory pressures as this is not a NURBS or BoT model, which use a LOT more memory, but it does at least use about 1.3GB.
Sean (Jun 30 2021 at 06:38):It'll still load fast, but you'll probably find some commands get noticably slower (e.g., killtree on just one of the m35 copies in there is excrutiatingly slow... do not try it on any of the levels or you may end up waiting a long whilte).
Vikram Atreya (Jun 30 2021 at 06:40):So I shoud test this by storing the names of all the objects and then taking note of the time and space used
Sean (Jun 30 2021 at 06:41):It depends
Sean (Jun 30 2021 at 06:42):You were talking about maybe storing the name of every object before a command runs. This type of db will tell you quickly if that is at all feasible.
Vikram Atreya (Jun 30 2021 at 06:43):Sean said:
You were talking about maybe storing the name of every object before a command runs. This type of db will tell you quickly if that is at all feasible.
Yes
Sean (Jun 30 2021 at 06:43):It's a good test case to make sure undo doesn't have some fundamental limitation
Vikram Atreya (Jun 30 2021 at 06:46):My bad, but could you reiterate the task(s) that I have to do next :sweat_smile:
This check for name stroing is a pretty small task, so what can i do after that
Sean (Jun 30 2021 at 06:48):A good test object in there is "suspension" which is one of the corner vehicles with about 400 regions and 400 primitives.
Sean (Jun 30 2021 at 06:51):Tasks next: 1) First, run this sequence -- make+make+kill+make+kill+kill -- and then make sure you can undo+undo+undo+undo+undo and end up with just the first make
Sean (Jun 30 2021 at 06:53):2) If you're still using _GLOBAL, switch to using some other convention. ideally, make it so undo command knows how many undo actions there are and/or what the next one is
Sean (Jun 30 2021 at 06:54):3) get killtree working so a single undo undoes the whole kill
Vikram Atreya (Jun 30 2021 at 18:29):Doing task 2 before task 1 will be better in my opinion because the no of undos, last change data will have to be stored in the new place/convention. If I do task 1 again I will have to rewrite code of undo according to the new convention
Vikram Atreya (Jun 30 2021 at 19:11):An idea is to store a stack/array as the data of a binary object and have it contain all the information on undos
Vikram Atreya (Jun 30 2021 at 19:18):The stack could simply be a char** with memory being reallocated whenever we add or pop events
Vikram Atreya (Jun 30 2021 at 19:37):I just realized that the history command is doing something very similar to what we want to achieve by the undo stack
Vikram Atreya (Jun 30 2021 at 20:10):The best place that I can see to implement this stack data struc (similar to mged_hist) is include/ged/defines.h
Vikram Atreya (Jun 30 2021 at 20:21):bu_list would be just sufficient but knowing the length of the list will help in checking if the no of undos requested are actually possible. 2 options here:
Vikram Atreya (Jul 01 2021 at 04:25):I hadn't properly seen the attributes of bu_list. Since it does not have an attribute to store data, I have created a new DS and will implement multiple undo using it
Vikram Atreya (Jul 01 2021 at 21:00):I have implemented a new data struc in ged.h
struct ged_undo_stack {
struct bu_list l;
struct bu_attribute_value_set cmd_data;
int64_t n; /* no of undos posssible */
};
Using this I implemented undo for move
Vikram Atreya (Jul 01 2021 at 21:01):Im packing this in a binary object and then recovering this struct when undo is passed
Vikram Atreya (Jul 01 2021 at 21:01):I have a doubt in the recovery
Vikram Atreya (Jul 01 2021 at 21:02):struct ged_undo_stack *gusp;
gusp = (struct ged_undo_stack *)bu_malloc(binunif->count, "");
Is this correct code or will it lead to errors?
The basic tests that I did worked but I'm not sure if this is robust code
Vikram Atreya (Jul 02 2021 at 16:31):struct ged_undo_stack {
struct bu_list l;
struct bu_attribute_value_set cmd_data;
int64_t n; /* no of undos posssible */
};
I think this structure will not work because bu_list l is just a node, and the whole list is not being stored
Vikram Atreya (Jul 03 2021 at 19:55):I want to store a resizeable array of bu_attribute_value_set, to store the cmds and required info for the undo stack
but Im a little confused about how to allocate memory for this
Vikram Atreya (Jul 03 2021 at 21:16):Im kind of stuck on how to implement this stack which I can entirely store in the binary object
Vikram Atreya (Jul 03 2021 at 21:17):bu_list doesnt work since we need to pass all the data and storing many structs into 1 binary object and then recovering doesnt work out
Vikram Atreya (Jul 03 2021 at 21:19):A simple implementation could be just using a char* array, but I think storing bu_attribute_value_sets can be more functional and easy to deal with once setup
Vikram Atreya (Jul 03 2021 at 21:20):@Sean any hints here on how i can implement the DS
Vikram Atreya (Jul 05 2021 at 16:27):Also it would be great if you can give me weekly targets, because I feel progress has slowed down a bit over the last 1 week
Vikram Atreya (Jul 05 2021 at 20:04):@Sean is it a good idea to use 2D bu_vls arrays to store the undo information? Im not able to figure out what would be the best way to create a stack which can be stored in a bin obj and then recovered
Sean (Jul 06 2021 at 05:45):@Vikram Atreya apologies on the delay, it was a big holiday weekend. sounds like you've made a lot of progress and discoveries... and are knee deep in completely new concepts. No worries, though, as you are on the right track.
Sean (Jul 06 2021 at 05:46):taking a step back, the reason I mentioned #1 vs #2 was to separate the change of undo recording from the change of undo storage. It doesn't matter which you do first, but you really should just tackle one of the two concepts.
Sean (Jul 06 2021 at 05:54):you will want to do both, but I really suggest not trying to solve both at the same time, which you are/were attempting looking at bu_list and friends. just using a char array was a good redirect, but an avs could work IFF you have a plan for what the attribute names/convention will be for keeping track of things.
Sean (Jul 06 2021 at 05:56):that said, that's still really doing 1+2 again... so don't get caught up with that. Worst case, to do #2 first, you could/should implement exactly what you were doing in _GLOBAL (i.e., 3 strings) in memory. Even getting that working is going to present a couple challenges.
Sean (Jul 06 2021 at 05:58):Vikram Atreya said:
Sean is it a good idea to use 2D bu_vls arrays to store the undo information? Im not able to figure out what would be the best way to create a stack which can be stored in a bin obj and then recovered
if you pretend brl-cad's data structures don't exist, what is it you're trying to do??
Vikram Atreya (Jul 06 2021 at 05:59):I want to store a stack of string pairs
Sean (Jul 06 2021 at 05:59):until you figure that out, the rest is inconsequential and can be incredibly distracting as you're not familiar with the structures. bu_list can be made to work perfectly. bu_attribute_value_sets can be made to work. char *'s can be made to work...
Vikram Atreya (Jul 06 2021 at 06:00):a stack of vector of string pairs is even better
Sean (Jul 06 2021 at 06:00):what are the string pairs?
Vikram Atreya (Jul 06 2021 at 06:01):examples are
last changed object : obj1
moved object old name: old_name etc
Sean (Jul 06 2021 at 06:03):data-type wise, what are they?
Vikram Atreya (Jul 06 2021 at 06:04):char*
Sean (Jul 06 2021 at 06:04):so that works for the object name, but what about it's pair?
Vikram Atreya (Jul 06 2021 at 06:05):I was thinking of an array of size 2
Sean (Jul 06 2021 at 06:06):you mean a <char, char> pair?
Sean (Jul 06 2021 at 06:06):char *
Vikram Atreya (Jul 06 2021 at 06:06):Yes
Sean (Jul 06 2021 at 06:07):In your example, "last changed object" is a lot of senseless bytes to be storing that anywhere...
Vikram Atreya (Jul 06 2021 at 06:08):Okayy
Vikram Atreya (Jul 06 2021 at 06:08):So we decide on positions previously itself and then assign values?
Sean (Jul 06 2021 at 06:09):positions??
Vikram Atreya (Jul 06 2021 at 06:09):Like index 0, will be last command, second might be the object we are making, old object name, object we are killing based on the prev index value
Sean (Jul 06 2021 at 06:11):that's essentially itemizing the second pair element in your description, the <..., string> value
Sean (Jul 06 2021 at 06:11):but what I think you're getting at is also storing the "action" with it
Sean (Jul 06 2021 at 06:12):which would probably be more appropriate as an enumeration, an ID of sorts, so the pairing is <int, string>
Sean (Jul 06 2021 at 06:15):So that said, why don't you start there with exactly that as simply as possible, with as few changes as possible.
Sean (Jul 06 2021 at 06:15):what I mean is don't worry about the in-memory data structures .. that's #2 and that can be later. you can focus solving the undo logic, not the in-memory mechanism.
Sean (Jul 06 2021 at 06:16):what I mean by that is look at how you're using _GLOBAL ... what changes did you make there?
Vikram Atreya (Jul 06 2021 at 06:16):Sean said:
what I mean by that is look at how you're using _GLOBAL ... what changes did you make there?
there = undo code?
Vikram Atreya (Jul 06 2021 at 06:16):or changes to _GLOBAL?
Sean (Jul 06 2021 at 06:16):how are you using _GLOBAL ?
Vikram Atreya (Jul 06 2021 at 06:17):Im storing information in attribute pairs
Sean (Jul 06 2021 at 06:17):more specific
Vikram Atreya (Jul 06 2021 at 06:18):For any addition of object--
last_change: created
last_created_object: its_name
Vikram Atreya (Jul 06 2021 at 06:18):For move:-
last_change: move
mv_old_name: old_name
mv_new_name: new_name
Vikram Atreya (Jul 06 2021 at 06:19):Kill and change are similar to make in terms of data stored
Sean (Jul 06 2021 at 06:19):this is why talking about bu_list's and such is a HUGE leap... :)
Sean (Jul 06 2021 at 06:20):okay, so you have a whole slew of attribute names and values, and they're apparently command-specific
Vikram Atreya (Jul 06 2021 at 06:21):Theres only 4 types as of now in which I have accommodated all cmds Ive worked on till now
Vikram Atreya (Jul 06 2021 at 06:21):Sean said:
okay, so you have a whole slew of attribute names and values, and they're apparently command-specific
but yes
Sean (Jul 06 2021 at 06:23):so there's already a problem to consider ... how does move fit into the pair paradigm?
Sean (Jul 06 2021 at 06:23):because I see <int, string, string> with old_name and new_name ... are both needed?
Vikram Atreya (Jul 06 2021 at 06:24):Sean said:
because I see <int, string, string> with old_name and new_name ... are both needed?
Yeah both names will be needed to stored somewhere to properly undo the change
Sean (Jul 06 2021 at 06:26):so you have an action that is <UNDO, string, string> ... and <CHANGED, string>... what else data-wise?
Vikram Atreya (Jul 06 2021 at 06:27):Sean said:
so you have an action that is <UNDO, string, string> ... and <CHANGED, string>... what else data-wise?
for the move command that's it I would say
Sean (Jul 06 2021 at 06:27):note that this is no longer a "pair" in CS terms, I'm just referring to an abstract tuple concept at the moment
Sean (Jul 06 2021 at 06:27):sorry, <MOVE, string, string>
Sean (Jul 06 2021 at 06:30):so what else is there?
Sean (Jul 06 2021 at 06:30):itemize all the info being stored in _GLOBAL that way
Vikram Atreya (Jul 06 2021 at 06:32):For any addition of object--
last_change: created
last_created_object: its_name
For move:-
last_change: move
mv_old_name: old_name
mv_new_name: new_name
For kill:-
last_change: kill
last_killed: killed_object
For object modifiication:
last_change: modify
last_modified: modified_object
Sean (Jul 06 2021 at 06:33):now convert that into tuples...
Vikram Atreya (Jul 06 2021 at 06:34):<CREATE, string>
<MOVE, string, string>
<KILL, string>
<MODIFY, string>
Sean (Jul 06 2021 at 06:39):that's great, so let's go with the simplest route and consolidate
Sean (Jul 06 2021 at 06:40):since MOVE is an outlier, lets ignore it for the moment
Sean (Jul 06 2021 at 06:40):you can, however, easily consolidate CREATE, KILL, and MODIFY
Sean (Jul 06 2021 at 06:41):using a simple convention, you can combine them into a single field. so I suggest trying that next.
Vikram Atreya (Jul 06 2021 at 06:45):Sean said:
using a simple convention, you can combine them into a single field. so I suggest trying that next.
Okay, like all this data in 1 string?
Vikram Atreya (Jul 06 2021 at 06:45):with a recognizable separator?
Sean (Jul 06 2021 at 06:45):without a separator I'd think, but yes -- 1 string
Sean (Jul 06 2021 at 06:46):using just 1 avs, change create, kill, and modify into one string
Sean (Jul 06 2021 at 06:47):they're all identical, so you can always get any of them by iterating two at a time.
Sean (Jul 06 2021 at 06:47):action: int string int string int string
Vikram Atreya (Jul 06 2021 at 06:50):Sean said:
using just 1 avs, change create, kill, and modify into one string
Why do we need an avs now? if we have only 1 string?
Vikram Atreya (Jul 06 2021 at 06:50):Okay for the undo no and then action?
Sean (Jul 06 2021 at 06:51):"last_change: created" is an avs
Sean (Jul 06 2021 at 06:53):again, making as few changes as possible so we don't get lost in complexity... where/how are you going to store a list of actions like "action: int string int string int string" ?
Sean (Jul 06 2021 at 06:55):how are you storing "last_change: kill" ?
Vikram Atreya (Jul 06 2021 at 06:56):Sean said:
"last_change: created" is an avs
avs is an attribute value set
So it can store a lot of pairs
Now that we have 1 command stored just in 1 string, eg:MAKE_obj1, KILL_obj2, CHANGE_obj3
That will be the value, and the key is the undo no like 1 for first undo?
Sean (Jul 06 2021 at 06:58):sorry, I'm being loose with terminology -- I meant it's an"avp", it's not a set. _GLOBAL is effectively an avs (or more technically, it has an avs)
Vikram Atreya (Jul 06 2021 at 06:59):So we have many avps for 1 cmd or only 1 avp?
Sean (Jul 06 2021 at 07:00):don't get lost....
how are you storing "last_change: kill" ?
Vikram Atreya (Jul 06 2021 at 07:02):Sean said:
don't get lost....
how are you storing "last_change: kill" ?
Im lost already :grimacing:
I did not get your question, Im just storng it as a pair in _GLOBAL' avs
Sean (Jul 06 2021 at 07:02):yes, but HOW
Vikram Atreya (Jul 06 2021 at 07:03):as a pair of strings I suppose
Sean (Jul 06 2021 at 07:03):no.. no
Sean (Jul 06 2021 at 07:03):what line of code make it store that pair into _GLOBAL?
Vikram Atreya (Jul 06 2021 at 07:03):Okay
Sean (Jul 06 2021 at 07:03):that's how...
Vikram Atreya (Jul 06 2021 at 07:04):So first I extract the avs from global, store a copy locally, add the attributes I want to add and then I replace the avs in _GLOBAL with the copy I made
Sean (Jul 06 2021 at 07:04):how do you do that last step?
Sean (Jul 06 2021 at 07:04):and/or the second step
Vikram Atreya (Jul 06 2021 at 07:05):there is a function db5_replace_attributes()
Sean (Jul 06 2021 at 07:05):how do you add the attributes you want to add?
Vikram Atreya (Jul 06 2021 at 07:05):Using bu_avs_add
Sean (Jul 06 2021 at 07:06):okay, so that's your next step....
Sean (Jul 06 2021 at 07:06):how are you going to store a list of actions like "action: int string int string int string"?
Vikram Atreya (Jul 06 2021 at 07:07):make them into 3 avps and then add them to the avs, have a marker in the key that all of them belong to the same action
Sean (Jul 06 2021 at 07:08):no good
Sean (Jul 06 2021 at 07:08):find a solution that is just 1 avp
Vikram Atreya (Jul 06 2021 at 07:10):Can be done, we put a separator like double undo or | in the string and store them as a single string in the avp's value
Sean (Jul 06 2021 at 07:10):what purpose does the separator serve?
Vikram Atreya (Jul 06 2021 at 07:11):To distinguish between different data
Vikram Atreya (Jul 06 2021 at 07:11):for eg, MOVE|old_name|new_name
Sean (Jul 06 2021 at 07:11):stop it.. we're ignoring MOVE...
Sean (Jul 06 2021 at 07:12):keep it simple...
Vikram Atreya (Jul 06 2021 at 07:12):Okayy :sweat_smile:
Vikram Atreya (Jul 06 2021 at 07:14):the value could be ACTION|int|string|int|string|int|string
Sean (Jul 06 2021 at 07:15):what is ACTION?
Sean (Jul 06 2021 at 07:15):"action" was going to be the name, so I'm not sure why you'd put it in the value too...
Vikram Atreya (Jul 06 2021 at 07:16):Sean said:
"action" was going to be the name, so I'm not sure why you'd put it in the value too...
Shouldnt have put it
Vikram Atreya (Jul 06 2021 at 07:17):My bad
Sean (Jul 06 2021 at 07:17):okay, so then i'm back to saying you don't need a delimiter
Vikram Atreya (Jul 06 2021 at 07:20):Let me think of another method then, if there were no numbers in the string then there would be no need otherwise.... :thinking:
Sean (Jul 06 2021 at 07:20):at least not now, not yet. just complicates it.. if the value is "1 obj2 3 obj2 1 obj4 2 obj3" is that ambiguous?
Vikram Atreya (Jul 06 2021 at 07:20):No
Sean (Jul 06 2021 at 07:20):so go with something that simple then
Vikram Atreya (Jul 06 2021 at 07:21):Sean said:
at least not now, not yet. just complicates it.. if the value is "1 obj2 3 obj2 1 obj4 2 obj3" is that ambiguous?
but we do have a separator that is a space in this example right?
Sean (Jul 06 2021 at 07:22):sure, and we're assuming object names don't have spaces for now too
Sean (Jul 06 2021 at 07:22):that's okay
Vikram Atreya (Jul 06 2021 at 07:22):So I'll just use spaces between different data
Sean (Jul 06 2021 at 07:22):you'll have to get the avp to add new values to it, but you can continually add easily. undo can read from that and remove the last two values
Sean (Jul 06 2021 at 07:23):so try doing that so that kill, create, and change all add to that avp, and undo removes the last one ...
Sean (Jul 06 2021 at 07:23):disable the move code if it causes problems
Sean (Jul 06 2021 at 07:23):it's not the focus
Vikram Atreya (Jul 06 2021 at 07:26):Just to confirm
So every command adds to the same avs(to be precise) and undo keeps popping things from it?
Sean (Jul 06 2021 at 07:26):once you get it working, I suggest putting the code that adds/removes from the avp into a function. make the kill/create/change functions call those functions
Sean (Jul 06 2021 at 07:26):adds to the same avp
Sean (Jul 06 2021 at 07:27):an "action" avp
Sean (Jul 06 2021 at 07:27):or whatever you want to name it
Sean (Jul 06 2021 at 07:27):no other avp's
Sean (Jul 06 2021 at 07:27):make sense?
Sean (Jul 06 2021 at 07:28):once you do that, you should be able to implement an "undo list" subcommand that lists all the actions that can be undone...
Vikram Atreya (Jul 06 2021 at 07:29):Sean said:
an "action" avp
Sorry for getting confused
So one action avp can contain many commands?
for eg, an action might have make+kill+kill
and its avp comes out to be
ACTION: 1 obj1.s 2 obj2.s 2 obj3.s
Sean (Jul 06 2021 at 07:32):right
Sean (Jul 06 2021 at 07:33):Do that, and you will have succinctly solved #1
Vikram Atreya (Jul 06 2021 at 07:33):Yes got it
Vikram Atreya (Jul 07 2021 at 20:14):I have implemented undo for a multi-command action
eg,
We have few objects when we open the db, next we do "make+make+kill+make+make" , and then call undo
It undoes all changes since we have kept track i.e. only the initial objects are left
Vikram Atreya (Jul 07 2021 at 20:21):Right now a single undo removes all changes, I will reconfigure this to 1 change per undo
Vikram Atreya (Jul 07 2021 at 20:33):Vikram Atreya said:
Right now a single undo removes all changes, I will reconfigure this to 1 change per undo
Done!
Vikram Atreya (Jul 07 2021 at 20:52):Sean said:
2) If you're still using _GLOBAL, switch to using some other convention. ideally, make it so undo command knows how many undo actions there are and/or what the next one is
I will proceed to the next task then, for this implementing a new struct with char** and no_of_undos should suffice, where the char** can store the action strings
What are your thoughts @Sean ?
Sean (Jul 10 2021 at 03:28):@Vikram Atreya I'd recommend you tackle #1.5 next ... account for the remaining attributes you were storing on _GLOBAL (i.e., rename) ... figure out a way for handling that, ideally without using additional delimiters or command-specific logic.
Sean (Jul 10 2021 at 03:30):curious how you're thinking to handle it. I can think of a few ways, one in particular...
Vikram Atreya (Jul 10 2021 at 03:32):The first thing that comes to mind is a command-specific logic where, lets say 3 represents rename, then we iterate till we find 2 object names separated by a space.
Sean (Jul 10 2021 at 03:33):right, that's an option, but as you note it has a huge downside that the parsing logic has to suddenly become command-aware, it has to know that rename takes 3
Sean (Jul 10 2021 at 03:33):it also can't instantly know how many undo actions remain without inspecting all the command labels to know how many to skip.
Sean (Jul 10 2021 at 03:34):any other ideas?
Vikram Atreya (Jul 10 2021 at 03:35):Another idea could be keep iterating till we find the next number
Sean (Jul 10 2021 at 03:36):eh? how's that implement support for undoing rename?
Vikram Atreya (Jul 10 2021 at 03:36):For rename we need to store 2 strings unlike make/kill
Sean (Jul 10 2021 at 03:37):Not true...
Sean (Jul 10 2021 at 03:37):that's not a need, that's just how you're currently handling it
Vikram Atreya (Jul 10 2021 at 03:37):Rn Im storing the action value as "1 obj1.s 2 obj2.s 1 obj3.s ..... " where 1 represents make and 2 represents kill
Vikram Atreya (Jul 10 2021 at 03:38):Sean said:
that's not a need, that's just how you're currently handling it
Okay, let me think with only 1 string
Sean (Jul 10 2021 at 03:38):yep, sounds good
Vikram Atreya (Jul 10 2021 at 03:39):Vikram Atreya said:
Rn Im storing the action value as "1 obj1.s 2 obj2.s 1 obj3.s ..... " where 1 represents make and 2 represents kill
So if i stored 3 obj2.s obj5.s in this, to not make it command specific, I keep iterating and storing any number of words till I find the next no or the string ends
Sean (Jul 10 2021 at 03:40):that's not really any different
Vikram Atreya (Jul 10 2021 at 03:41):Sean said:
eh? how's that implement support for undoing rename?
I was replying to this
Sean (Jul 10 2021 at 03:42):well, yeah, but you said that's another idea ... that's the same idea. that's literally how you'd have to handle it, but it has problems that I mentioned.
Sean (Jul 10 2021 at 03:42):the parsing logic has to be aware
Sean (Jul 10 2021 at 03:43):I mean I get what you're saying, that it doesn't need to know it's a 3 and just keeps looking for a number, but that's still not deterministic. It's a linear cost operation O(n) to figure out what a given field entry is...
Sean (Jul 10 2021 at 03:44):you can't look at a stream like "1 foo 2 bar 3 foo bar 2 foo 1 bar 3 bar foo 1 foo 2 bar" and tell me whether the 7th argument is an object name or an undo identifier without parsing everything that came before it
Vikram Atreya (Jul 10 2021 at 03:44):Yes.
Sean (Jul 10 2021 at 03:46):whereas if it was always pairs "1 foo 2 bar 3 foo 2 foo 1 bar 3 bar 1 foo 2 bar", the 7th is odd, so that's always an identifier and is constant time O(1)
Sean (Jul 10 2021 at 03:47):so how can you implement undo support for renames and still remain constant time?
Vikram Atreya (Jul 10 2021 at 03:47):using a hash we can separate both names
Sean (Jul 10 2021 at 03:48):hm? what're you suggesting?
Vikram Atreya (Jul 10 2021 at 03:49):obj_name1_hash_obj_name2
Sean (Jul 10 2021 at 03:49):explain...
Vikram Atreya (Jul 10 2021 at 03:50):A separator (kinda), let me give an example
Vikram Atreya (Jul 10 2021 at 03:50):obj1.s_sep!#asdjfhfjnrbs#!!_obj2.s
Sean (Jul 10 2021 at 03:52):so I think you're suggesting you generate a random string and use that as a separator between the two object names?
Vikram Atreya (Jul 10 2021 at 03:52):Not a random string every time, some fixed string, and yes
Sean (Jul 10 2021 at 03:52):why does it need to be encoded/hashed/long like that?
Sean (Jul 10 2021 at 03:53):why not obj1.s|obj2.s or something similar?
Vikram Atreya (Jul 10 2021 at 03:53):Sean said:
why does it need to be encoded/hashed/long like that?
To avoid that pattern occurring in the object name
Vikram Atreya (Jul 10 2021 at 03:54):Sean said:
why not obj1.s|obj2.s or something similar?
Imnt sure but cant "|" be put in an object name?
Sean (Jul 10 2021 at 03:54):there are a number of chracters that are incredibly inconvenient/difficult to have in object names that you could use
Sean (Jul 10 2021 at 03:56):you could also use quoting rules to encode the pairing like /obj1.s/obj2.s/ and it would recognize the use of / or , or whatever else being used as a separator
Sean (Jul 10 2021 at 03:56):so that's an option, and not a terrible one, but not great too because we're now similarly no longer deterministic on the even number arguments that were previously all object names (or former object names)
Sean (Jul 10 2021 at 03:58):so keep that one in your back pocket, it could work but I think we can do better. The real issue is that rename is introducing command-specific behavior and we're scrambling to come up with awkward ways to encode it instead of trying to find a way to handle it like everything else
Vikram Atreya (Jul 10 2021 at 04:00):If we were to handle it like other commands, we will need a new place to store the old_name, could be in the attributes of the object itself but again that will be a pain for multiple renames
Sean (Jul 10 2021 at 04:00):think about what rename is doing and other ways you might handle it without code having logic specific to rename anywhere (i.e., 3 != "rename").
Sean (Jul 10 2021 at 04:00):Yes! excellent observation
Sean (Jul 10 2021 at 04:01):you could just have 3 = rename and "3 foo" might be "we renamed foo" and in the backup "foo" object, it has an attribute like "renamed_from" = "bar" where we could keep track of the additional/former name
Vikram Atreya (Jul 10 2021 at 04:02):Yes this can totally be done
Sean (Jul 10 2021 at 04:02):that's better and makes the undo parsing completely deterministic, but I think you're right .. it's potentially a pain for multiple renames... maybe.
Vikram Atreya (Jul 10 2021 at 04:02):Not if we are using the backup_object
Vikram Atreya (Jul 10 2021 at 04:02):Then every new name will have a different backup object
Vikram Atreya (Jul 10 2021 at 04:03):Only problem is when object is renamed to new_name and then again back to old_name
Sean (Jul 10 2021 at 04:03):sure, because nothing says the backup object's name has to have meaning. it could be "3 jjk2h34hk24k12kj1h5" and in that object, it has the from and to names recorded
Vikram Atreya (Jul 10 2021 at 04:03):This can be solved later on when by adding the undo_step_no to all backup objects
Sean (Jul 10 2021 at 04:04):so that's good and if we had lots of time, I might say do that next, but let me hint you towards another solution
Sean (Jul 10 2021 at 04:04):consider what rename does...
Sean (Jul 10 2021 at 04:05):I'd argue you can encode it already without introducing a new undo code. do you see why?
Vikram Atreya (Jul 10 2021 at 04:06):Sean said:
I'd argue you can encode it already without introducing a new undo code. do you see why?
Encode the backup object or in the action value string?
Sean (Jul 10 2021 at 04:06):nope
Sean (Jul 10 2021 at 04:07):I mean using the convention you have already where 1=make and 2=kill
Vikram Atreya (Jul 10 2021 at 04:07):Ohh yes
Vikram Atreya (Jul 10 2021 at 04:08):Make object with old_name and kill the one with the new name
Sean (Jul 10 2021 at 04:08):right! now what's the problem with that?
Vikram Atreya (Jul 10 2021 at 04:09):Extra space to store the backup object
Sean (Jul 10 2021 at 04:09):meh, don't care about that
Vikram Atreya (Jul 10 2021 at 04:09):or extra information required to know which object to make a copy of
Sean (Jul 10 2021 at 04:09):the suggestion is "rename foo bar" can be encoded as "1 bar 2 foo" right? what's the problem with that?
Vikram Atreya (Jul 10 2021 at 04:10):Sean said:
the suggestion is "rename foo bar" can be encoded as "1 bar 2 foo" right? what's the problem with that?
Yeah, but to make bar we need to either make a copy of an existing object or get it from a backup that we stored
Sean (Jul 10 2021 at 04:12):er, sure, but you have that... rename command has foo so it could define bar by copying foo
Sean (Jul 10 2021 at 04:12):and then store that copy as the backup object
Sean (Jul 10 2021 at 04:12):that's not a problem
Sean (Jul 10 2021 at 04:12):what is a problem is ...
Sean (Jul 10 2021 at 04:12):what happens when the user calls "undo"?
Sean (Jul 10 2021 at 04:13):and your action string is "1 bar 2 foo"
Vikram Atreya (Jul 10 2021 at 04:14):Sean said:
and your action string is "1 bar 2 foo"
Search for a backup for bar, make it and then kill foo
Sean (Jul 10 2021 at 04:14):not quite.. it wouldn't know to do all that...
Sean (Jul 10 2021 at 04:15):"undo" is going to just undo "2 foo"
Vikram Atreya (Jul 10 2021 at 04:15):Okay, yes
Sean (Jul 10 2021 at 04:15):i.e., the kill is undone, then a second undo will undo the make (by killing bar)
Sean (Jul 10 2021 at 04:16):so in that respect, undo is not terribly different from killall ..
Sean (Jul 10 2021 at 04:18):what's action type 3 ?
Sean (Jul 10 2021 at 04:19):1=make, 2=kill, 3=?
Vikram Atreya (Jul 10 2021 at 04:20):Sean said:
1=make, 2=kill, 3=?
not decided?
Vikram Atreya (Jul 10 2021 at 04:21):it could be combination cmd like g
Vikram Atreya (Jul 10 2021 at 04:21):or rather a change
Sean (Jul 10 2021 at 04:21):oh, so you didn't do what we talked about earlier then...
Sean (Jul 10 2021 at 04:22):Sean said:
you can, however, easily consolidate CREATE, KILL, and MODIFY
using a simple convention, you can combine them into a single field. so I suggest trying that next.
Vikram Atreya (Jul 10 2021 at 04:23):I havent dealt with MODIFY yet in the code, but can be done in a few min
Sean (Jul 10 2021 at 04:23):okay, no worries, but lets pretend you've done that :)
Sean (Jul 10 2021 at 04:24):so 3=change or whatever
Sean (Jul 10 2021 at 04:26):so yeah, assuming that, how would you encode "killall foo" where bar=obj1 u obj2 - foo
Sean (Jul 10 2021 at 04:28):using 1/2/3 ...
Vikram Atreya (Jul 10 2021 at 04:29):1 foo 3 comb_obj
Vikram Atreya (Jul 10 2021 at 04:29):or modify more comb objects if necessary
Sean (Jul 10 2021 at 04:29):what's comb_obj?
Vikram Atreya (Jul 10 2021 at 04:29):it could be any combination object which had a reference to foo before killall
Sean (Jul 10 2021 at 04:31):it's not unknown, I gave all the db objects.. the db has objects: foo bar obj1 obj2
Sean (Jul 10 2021 at 04:31):so killall foo does what?
Sean (Jul 10 2021 at 04:32):assume obj1 and obj2 are primitives
Vikram Atreya (Jul 10 2021 at 04:34):obj1 u obj2 - foo
i did not get this
what is the relation between obj1 and bar
Sean (Jul 10 2021 at 04:34):that's the CSG recipe. it just means bar is a combination and it references foo, so it will be modified by killall
Vikram Atreya (Jul 10 2021 at 04:35):3 bar 2 foo
Vikram Atreya (Jul 10 2021 at 04:36):This should be stored in the string
Sean (Jul 10 2021 at 04:42):excellent, right
Sean (Jul 10 2021 at 04:42):so you see then if you undo, that would only first undo 2 foo, which is only a partial undo of the killall command
Sean (Jul 10 2021 at 04:42):what's needed to fix that?
Sean (Jul 10 2021 at 04:42):or how would you fix that for killall? ... without introducing logic that is specific to killall
Vikram Atreya (Jul 10 2021 at 04:44):Just asking, is there a plan to undo the whole action all at once in the future and have multiple actions where 1 action was just 1 command?
kill all would have action string as "3 bar1 3 bar2 3 bar3 2 foo" and 1 undo would undo all this
Sean (Jul 10 2021 at 04:47):heh
Sean (Jul 10 2021 at 04:48):OF COURSE that's the plan... :) otherwise that's not really undo support for commands like undo.
Vikram Atreya (Jul 10 2021 at 04:49):Sean said:
or how would you fix that for killall? ... without introducing logic that is specific to killall
but right now Idk, I'm thinking
Sean (Jul 10 2021 at 06:02):assuming you're still thinking about it @Vikram Atreya ... here's a suggestion: keep the same 1/2/3 action codes, but make 0 be a set / grouping action and let the object that follows be an attribute object (just like _GLOBAL) that contains the list of things to undo for that set.
Sean (Jul 10 2021 at 06:04):so in our db with four objects, maybe the undo history looks like this: "1 foo 1 obj1 1 obj2 1 obj3 1 bar 3 bar 2 obj3"
Sean (Jul 10 2021 at 06:06):then you run "killall foo", and the undo string in _GLOBAL looks like: "1 foo 1 obj1 1 obj2 1 obj3 1 bar 3 bar 2 obj3 0 killtree_2b4a7c20"
Sean (Jul 10 2021 at 06:07):and the object killtree_2b4a7c20 has an action = "3 bar 2 foo"
Sean (Jul 10 2021 at 06:08):so running undo pops off the 0 action, reads the object, and recursively handles undoing the entire action list in killtree_2b4a7c201
Sean (Jul 10 2021 at 06:10):once you get that working, make sure everything that accesses _GLOBAL directly or indirectly is going through a function, not directly accessing it from each command.
Vikram Atreya (Jul 10 2021 at 09:25):I had to go to the bank for some work, I got the idea; Will start working on it
Sean (Jul 12 2021 at 18:53):Any luck? You’ll probably have to figure out how to write an attribute object..
Vikram Atreya (Jul 12 2021 at 19:12):Sean said:
Any luck? You’ll probably have to figure out how to write an attribute object..
Was a lil busy with another project submission the past 2-3 days. Will update you on progress tomorrow for sure
Sorry for the delay
Vikram Atreya (Jul 14 2021 at 05:13):Sean said:
assuming you're still thinking about it Vikram Atreya ... here's a suggestion: keep the same 1/2/3 action codes, but make 0 be a set / grouping action and let the object that follows be an attribute object (just like _GLOBAL) that contains the list of things to undo for that set.
Can we have different numbers that mean a group
Vikram Atreya (Jul 14 2021 at 05:13):For eg, 4 is the code for move, and the object passed after it contains attributes related to the command
Vikram Atreya (Jul 14 2021 at 05:17):Working :
>mv obj1.s obj2.s
//An attribute object attr_hash is created with info about old name and new name
//4 attr_hash is appended to action's value
>undo
//code "4" is seen, it accesses attr_hash and using info in it undoes the move
curious why not zero? grouping is the first "meta" action representing a transaction, so it doesn't make sense in the middle of the numbering scheme
Sean (Jul 14 2021 at 18:34):consider if more actions or specializations get added (e.g., editing an object's attribute could be tracked separately more efficiently with 3 more attr_add/attr_remove/attr_change codes
Vikram Atreya (Jul 14 2021 at 20:35):Sean said:
curious why not zero? grouping is the first "meta" action representing a transaction, so it doesn't make sense in the middle of the numbering scheme
As I was implementing it, I realize 0 is a better choice. I had this doubt, would it be better if we could specify what command inside the attribute object, for eg, make the attr_obj have
command: move
old_name: obj1.s
new_name: obj2.s
or we only want to have a string like "2 obj1.s 1 obj2.s"
Vikram Atreya (Jul 14 2021 at 20:36):If we specify the command, it would make the logic more command-specific but will also be better in terms of space complexity
Vikram Atreya (Jul 14 2021 at 20:45):Also what is the best way to generate random names for these attribute objects, is there a built-in function?
Vikram Atreya (Jul 15 2021 at 18:30):Vikram Atreya said:
. . . specify what command inside the attribute object, for eg, make the attr_obj have
command: move
old_name: obj1.s
new_name: obj2.s
I have implemented this
Vikram Atreya (Jul 15 2021 at 18:33):@Sean is this method fine or should I implement it like action value sequence (1 obj1.s 2 obj2.s)
Sean (Jul 17 2021 at 07:35):Well, it greatly depends. I mean in general it doesn't (yet) matter ... but it will.
Sean (Jul 17 2021 at 07:37):@Vikram Atreya I would think you want to use the exact same parsing/handling logic that you have/had as this is the basis for recursive transactions.
Sean (Jul 17 2021 at 07:37):You're going to want to handle actions within actions within actions that need to be undone and the logic for handling that needs to be as simple as possible in order to be understandable, maintainable, particularly for others.
Sean (Jul 17 2021 at 07:39):Let me ask you, though, what made you want to not do "2 obj1.s 1 obj2.s" ?
Vikram Atreya (Jul 17 2021 at 15:19):Sean said:
Let me ask you, though, what made you want to not do "2 obj1.s 1 obj2.s" ?
No reason, 2 obj1.s 1obj2.s could be better as max memory over time would be lesser
Vikram Atreya (Jul 17 2021 at 15:21):Sean said:
Vikram Atreya I would think you want to use the exact same parsing/handling logic that you have/had as this is the basis for recursive transactions.
Understood, will make this change
Sean (Jul 18 2021 at 17:08):Part of the reason for making it it same is so that the parsing logic is exactly the same as was used on _GLOBAL, so there's no duplicate code
Sean (Jul 18 2021 at 17:09):Keep in mind that the string-based format is not the likely end-state. That's just to keep it simple for now. We'll want to change that to binary later.
Sean (Jul 18 2021 at 17:10):Binary begs for a different storage container or binary attributes, which are things that will require testing and rethinking, so they're deferred until later.
Sean (Jul 18 2021 at 17:11):(We have binary objects, but that code doesn't have any examples and needs to be enabled/tested.)
Vikram Atreya (Jul 18 2021 at 17:11):I'm trying to finish this recursive logic by today (with strings)
Sean (Jul 18 2021 at 17:11):The current string implementation is also flawed if an object name has spaces or quotes in it, but that's okay (for now).
Sean (Jul 18 2021 at 17:11):binary will fix that later.
Vikram Atreya (Jul 18 2021 at 17:55):Vikram Atreya said:
Also what is the best way to generate random names for these attribute objects, is there a built-in function?
@Sean ping?
Sean (Jul 18 2021 at 19:53):just about any name can work for now, maybe try using ged_make_name()? Could add a -r option if you really want something random. It generates a sequential name.
Sean (Jul 18 2021 at 19:53):so you could have it make something like .undo.###
Vikram Atreya (Jul 19 2021 at 18:05):Im refactoring a lot of code so that there very less redundant code, so it is taking some time. I will mostly complete the recursive function for executing actions today.
Sean (Jul 20 2021 at 06:25):@Vikram Atreya that's great, keep it up. Be sure to post updated code so I can check it out.
Sean (Jul 20 2021 at 06:27):On that note, I think we should also code deep and address that fundamental issue supporting objects with special spaces in the name. I suggest you introduce a separator, since that's pretty much the simplest thing you can do right now, at least that I can think of, to fix it.
Sean (Jul 20 2021 at 06:35):I think you still want "int[space]name" pairings so you can assume the number is the code, followed by whitespace, followed by the object name. I suggest something like '/' or ';' for the separator since they're problematic in names anyways.
Sean (Jul 20 2021 at 06:36):You will want to make sure the object names you're writing do not have that separator in them (and if they do, just print a message and abort the undo recording).
Sean (Jul 20 2021 at 06:37):If you don't know how to tokenize over a separator, it's something like this:
#include <stdio.h>
#include <string.h>
int main(int ac, char *av[]) {
char *str = strdup("1 foo;2 bar;0 asjdsdfjal");
char *token;
while ((token = strsep(&str, ";")) != NULL) {
printf("token is [%s]\n", token);
}
return 0;
}
I will make a PR soon.
Vikram Atreya (Jul 20 2021 at 20:46):I have implemented the recursive logic to execute "0 attr_obj"
Vikram Atreya (Jul 20 2021 at 20:46):So undo works for move now using the new logic
Vikram Atreya (Jul 20 2021 at 20:47):I have written functions to reduce redundant code, but still haven't replaced it at all places
and there's a lot of useless code commented across files, ill remove all that and make a PR tomorrow
Vikram Atreya (Jul 21 2021 at 20:31):Made a patch
Vikram Atreya (Jul 21 2021 at 20:50):I havent yet randomized the names of the attribute objects
Im also planning on making modify into a grouped cmd using 0, instead of having 3
Sean (Jul 22 2021 at 04:49):Not sure I follow what you mean there.. You talking abou the modify "code", switching it from 3 to instead being the same as grouping, i.e., 0?
Sean (Jul 22 2021 at 04:49):Not sure I see how that'd work.
Vikram Atreya (Jul 22 2021 at 04:54):Modify is nothing but kill the present object and recover old object
Vikram Atreya (Jul 22 2021 at 04:54):So cant it be grouped as a kill+make
Sean (Jul 22 2021 at 08:00):I suppose that's true, just as how a rename operation can also be thought of as a make+kill, but handling it specifically would be a likely optimization to reduce memory churn.
Sean (Jul 22 2021 at 08:01):Welcome to do it however you like but I'd suggest saving your time if it works.
Vikram Atreya (Jul 22 2021 at 08:06):Next I will focus on giving random names to attr objs and changing the code to store all backup objects as binary objects
rn only combination backup objects are stored as binay objects
Sean (Jul 24 2021 at 07:01):@Vikram Atreya did you finish all the other parts? that is, there is just one set of functions handling the storing/parsing of undo information from an attribute field (both _GLOBAL and stand-alone objects), attribute-only objects are working for groupings (case 0), code is refactored for all the commands from before, and undo is working with the recursive stores?
Sean (Jul 24 2021 at 07:12):if so, next step I think would help greatly will be some basic unit tests that succeed and fail. see src/libged/tests/test_tops.c for an example, but you'll want to make it call ged_undo() in various ways. e.g., undo with no db, undo with db but no prior commands, undo after make, N undos after N makes, undo after unsupported command, etc
Vikram Atreya (Jul 24 2021 at 08:03):Sean said:
Vikram Atreya did you finish all the other parts? that is, there is just one set of functions handling the storing/parsing of undo information from an attribute field (both _GLOBAL and stand-alone objects), attribute-only objects are working for groupings (case 0), code is refactored for all the commands from before, and undo is working with the recursive stores?
At the top of my head, I feel all of this has been done, but I have still left some code from the previous logic just in case, the idea was switched again. I will clean that up first and check for all the things you have mentioned.
Vikram Atreya (Jul 24 2021 at 08:03):I will get back to you tonight, once Im sure
Vikram Atreya (Jul 24 2021 at 21:22):Sean said:
Vikram Atreya did you finish all the other parts? that is, there is just one set of functions handling the storing/parsing of undo information from an attribute field (both _GLOBAL and stand-alone objects), attribute-only objects are working for groupings (case 0), code is refactored for all the commands from before, and undo is working with the recursive stores?
Yes, all of this is done. There is some small bug in the code, after I changed it to store binary objects for all killed objects. Solving that right now
Vikram Atreya (Jul 25 2021 at 10:41):when I store binary objects and try to recover them, I need to give it flags (SOLID/ COMB/ REGION). Can I found out what the object is from its external representation?
Sean (Jul 28 2021 at 12:54):@Vikram Atreya it depends on what you mean by external representation. if you serialized to a bu_external on-disk representation, then you'd get the type by cracking the header (getting a raw internal5).
Sean (Jul 28 2021 at 12:56):I'm a little confused as to what you changed though -- kill shouldn't need to encode anything I'd think.. having it just store the object that was deleted seems ideal so that recovery is as simple as possibly renaming the object or just changing an attribute or similar.
Sean (Jul 28 2021 at 12:57):what I thought you were working on was a way to story "case 0" groupings. how to store their string into an object as there is no existing object to hang that information off of. I suggested an attribute-only object, which is a special non-geometric object (_GLOBAL is an example... it's an object, but it's not geometry).
Vikram Atreya (Jul 28 2021 at 16:19):Sean said:
what I thought you were working on was a way to story "case 0" groupings. how to store their string into an object as there is no existing object to hang that information off of. I suggested an attribute-only object, which is a special non-geometric object (_GLOBAL is an example... it's an object, but it's not geometry).
Right now, I am using attribute objects only
Vikram Atreya (Jul 28 2021 at 16:19):Sean said:
I'm a little confused as to what you changed though -- kill shouldn't need to encode anything I'd think.. having it just store the object that was deleted seems ideal so that recovery is as simple as possibly renaming the object or just changing an attribute or similar.
this was regarding to combination objects, they were planned to be stored as binary objects and then recovered.
Vikram Atreya (Jul 28 2021 at 16:19):I thought of generalizing it to all objects
Vikram Atreya (Jul 29 2021 at 19:27):I have written a few tests for make, kill and move
Vikram Atreya (Jul 29 2021 at 19:27):I will also write tests for arb, mirror, etc tomorrow
Sean (Jul 30 2021 at 17:46):Vikram Atreya said:
I thought of generalizing it to all objects
That does sound good if it really can be generalized so there's one code for all objects, but probably not a problem if combinations are handled separately (unless that means other objects will then also need to be handled separately like dsp or extrude or other objects that refer to other objects by name).
Sean (Jul 30 2021 at 17:46):So if you can generalize it, great; but if not, that's probably okay too.
Vikram Atreya (Jul 30 2021 at 18:27):Yeah i'm generlaizing it
Vikram Atreya (Jul 30 2021 at 18:28):Also what is the target that you are expecting before the end of the coding period?
Sean (Jul 31 2021 at 01:45):er, undo working for all commands, no? :)
Vikram Atreya (Jul 31 2021 at 05:26):All 300+ commands?
Vikram Atreya (Aug 01 2021 at 19:20):Im right now focussing on killall, and a lot of simple commands can be dealt with quickly. but dbconcat might be a big challenge
Sean (Aug 02 2021 at 19:08):That's why it's important to take this one step at a time. Once a half dozen commands are handled properly, the remaining 294 should actually be really easy (or perhaps even automatic for all but a few).
Vikram Atreya (Aug 02 2021 at 19:13):So right now i should proceed with killall?
Vikram Atreya (Aug 02 2021 at 19:13):I will try to finish it by tomorrow
Vikram Atreya (Aug 03 2021 at 09:28):For killall, I can have code within killall.c also right?
Since only having it in the wrapper will not suffice?
Sean (Aug 03 2021 at 14:19):ideally killall and the other commands are changed to emit/record the actions taken, so they can be undone
Erik (Aug 03 2021 at 14:45):killall should be a chain of deletes that recurses dwon the tree, wouldn't just saving the deletes to journal be adequate? or does it need to be marked to go back several? (hehe, killall, the great linux vs unix litmus)
Vikram Atreya (Aug 03 2021 at 15:31):Sean said:
ideally killall and the other commands are changed to emit/record the actions taken, so they can be undone
Yes, so we write the changes to our action string, the code writing that event will be inside ged_killall_core
Vikram Atreya (Aug 04 2021 at 18:03):killall is proving to be slightly tough, imnt getting the idea of exactly how ill do it. is there anything else I could focus on for sometime? @Sean
Vikram Atreya (Aug 21 2021 at 18:22):For the final submission can I write the blog in brlcad's website?
Sean (Aug 21 2021 at 19:48):Absolutely @Vikram Atreya. We don't have a blog, but can post it as a page or put it in the wiki and we can showcase it on fb and our mailing list.
Vikram Atreya (Aug 21 2021 at 20:06):Sure, i will make a page on the wiki
Vikram Atreya (Aug 21 2021 at 20:06):Is there any example from previous years?
Vikram Atreya (Aug 21 2021 at 20:06):the gsoc archive page seems to be down
Vikram Atreya (Aug 23 2021 at 11:18):https://brlcad.org/wiki/User:Vikram_Atreya/GSoC21/Project_Report
This is the link I have submitted for final evaluations, do suggest changes if any
Sean (Aug 24 2021 at 05:27):I think that looks pretty good. Only suggestion would be to add a section with your thoughts on what's needed next, and what's needed to "finish" the effort -- for some definition of complete.
Vikram Atreya (Aug 24 2021 at 18:13):Sure will add it
Last updated: Nov 15 2024 at 00:49 UTC