Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XMLSerializer with visitor WIP #4536

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

cybercop23
Copy link
Collaborator

No description provided.

@cybercop23
Copy link
Collaborator Author

Started a new PR since I really screwed up the last one.

const Model* m = dynamic_cast<const Model*>(&arch);
const ModelManager& mgr = m->GetModelManager();
std::vector<std::string> groups = mgr.GetGroupsContainingModel(m);
std::vector<Model*> modelGroups = mgr.GetModelGroups(m);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@AzGilrock so I can get to the list of groups per your suggestion before, so I can create the group with the proper name and model (Arch in this case), but when I create the group itself, I need to pull in more info from the group itself, center, buffer, preview, tag color, etc... but I can't get it since that's a ModelGroup object vs teh ArchesModel object that I'm in. Dynamic casting doesn't work (at least couldn't figure out a way to do it).

Same for SubModel, but for that I created some more functions to return what I need, but still same issue... can't get from a type ArchesModel to SubModel or ModelGroups.

Any ideas/suggestions?

@AzGilrock
Copy link
Collaborator

I'm sure you can create a ModelGroup object if you have the list of model groups but I'm out of town at the Minnesota mini so can't give specific help till Sunday or Monday.

@cybercop23
Copy link
Collaborator Author

I'm sure you can create a ModelGroup object if you have the list of model groups but I'm out of town at the Minnesota mini so can't give specific help till Sunday or Monday.

No worries. I'll keep trying. Enjoy the Min.

@cybercop23
Copy link
Collaborator Author

okay.. i'm back on this.. so how can i convert from an arch model to model or submodel to get the rest of the relevant info?

@AzGilrock
Copy link
Collaborator

Pretty sure I showed you code to get the base model* and to the submodels over in the PR #4529

@cybercop23
Copy link
Collaborator Author

cybercop23 commented May 7, 2024

Okay.. I was passing the results of that into another function and that's what got me screwed up. I changed it to instead pass the model so that I can re-use the AddGroups for all models and I'm a bit further. But I'm sort of back where I was... that I can't (don't know how) get to the [ModelGroup] that's part of the "g" variable.

I call AddGroups from the Visit/Arches and pass the casted model (m). I then use that again within the AddGroups to get the list and create the groups. You can see in the output (top right) that I get the groups, and layout_attribute (from baseobj) but how can I get to the rest of the [ModelGroup] specific items?

image

I'm sure it is something simple like GetModel()->GetModelGroup() blah balh.. but I can't figure it out... :(

@AzGilrock
Copy link
Collaborator

You need to be creating a ModelGroup pointer if you want to access member variables of that class type. Looks like you have the list of groups being converted into a vector of Model* instead of ModelGroup*.

@cybercop23
Copy link
Collaborator Author

Yeah.. that's what tried and the GetModelGroups returns a Model not ModelGroup... This is what I keep getting stucked on.
image

@AzGilrock
Copy link
Collaborator

So you should,be able to cast a Model* to a ModelGroup* and check for nullptr to make sure it exists.

@cybercop23
Copy link
Collaborator Author

cybercop23 commented May 8, 2024

Arggg... why am i always one character off.... got it......

<modelGroup name="All" models="Arche1" LayoutGroup="Groups" selected="0" layout="Groups" GridSize="400" TagColour="#000000" centreMinx="0" centreMiny="0" centreMaxx="0" centreMaxy="0" centrex="-224.015060" centrey="950.592346" centreDefined="1" DefaultCamera="2D" XCentreOffset="0" YCentreOffset="0"/> \n <modelGroup name="Arches" models="Arche1" LayoutGroup="Default" selected="0" layout="Default" GridSize="400" TagColour="#FF0000" centreMinx="0" centreMiny="0" centreMaxx="0" centreMaxy="0" centrex="-469.535522" centrey="1084.479492" centreDefined="1" DefaultCamera="2D" XCentreOffset="0" YCentreOffset="0"/>

@cybercop23
Copy link
Collaborator Author

Here's the Arch model exported with model, submodels, groups and aliases at all levels.
Two things I just noticed - 1. You don't get prompted to choose and export groups - it just does it & 2. On Import although I just deleted the original model that was exported, the import picked up a new name (-2) and thus groups weren't updated as they should.

Will update the xmlserializer.h in a few, but if you can take a look and make sure things look good, I'll start at the other models.
Arch.txt

@AzGilrock
Copy link
Collaborator

Kinda hard to just look at an export and know if anything is missing or wrong. I would export with the latest version of code then compare against an export with your code. I don't even know about a prompt to ask about groups when exporting but less prompts the better.

@cybercop23
Copy link
Collaborator Author

Kinda hard to just look at an export and know if anything is missing or wrong. I would export with the latest version of code then compare against an export with your code. I don't even know about a prompt to ask about groups when exporting but less prompts the better.

Yeah.. that's what I've been doing.. exporting and importing to make sure they match. Once I have all models done (soon) I'll export my hole show and reimport it and diff the rgbeffects file.

I noticed, I'm getting this when casting.. should I be concered or any ways to address it? I don't get it for the ModelGroup cast:
image

@AzGilrock
Copy link
Collaborator

I believe "incomplete type" is usually the error when a header file is missing. I run into this when I forward declare a class in a header but forget to include the header file in the .cpp. But usually that issue would cause the compile to fail. Make sure the file is including whatever .h file defines Submodel. Also after any "dynamic_cast" I would perform a check to make sure the pointer is not nullptr.

@cybercop23
Copy link
Collaborator Author

Assuming no issues, all models with all parms changes export and match the existing rgb_effects file. (We already had submodels and aliases exported at all levels)

Added placeholder export fuunctions to all models, so I can export and check (whynot) and lots of functions to get the attributes needed.
We still need to look at 3layerscreen location since that's crapping out and disabled on most models - but still rgbeffects match.

Next are Faces and States - since they apply to all models.

@cybercop23
Copy link
Collaborator Author

I've added Faces and States and a custom sort function (needs to be added to all -I will do that) that sorts the attributes, but keeps name in the first position to make it easy to see/locate.

@cybercop23
Copy link
Collaborator Author

@AzGilrock Made some structural changes and tested all exports copied into one file as xlights_rgbeffects.xml and it loaded as expected... well almost....
The AddThreePointScreenLocationAttributes func fails on dynamic_cast for some models (CandyCane, ChannelBlock & SingleLine) and those are the ones that are diff on the new rgbeffects.xml. It is either the x2 or angle that's needed.
I tried to "fix" ChannelBlock, but my initial thought failed. You said you had some ideas.. Can you please review that part.
Once we correct that, we should have a clean way to use this new format for import/export and save rgbeffects.

@AzGilrock
Copy link
Collaborator

You don't add every ModelScreenLocation to every model. The ThreePointScreenLocation only applies to Arches, Candy Canes, and Icicles I believe.

@AzGilrock
Copy link
Collaborator

AzGilrock commented May 10, 2024

I took a quick look. There is still a ton of work to do. For each model you need to look at the class definition to figure out which screen location class it uses then you'll know which class attributes need to be added. For instance a TreeModel inherits from MatrixModel and that class is "class MatrixModel : public ModelWithScreenLocation(BoxedScreenLocation)" So that means it needs the BoxedScreenLocation attributes. That function does not exist yet. Also the ThreePointScreenLocation models will all need the TwoPointScreenLocation attributes. You want the functions to only add the attributes that are in each class.
Every model gets the AddModelScreenLocationAttributes because they are common to all.

So we need all the following new functions:
AddBoxedScreenLocationAttributes
AddTwoPointScreenLocationAttributes
AddPolyPointScreenLocationAttributes
and I'll need an AddMultiPointScreenLocationAttributes but I can add that when I make that model available.

Then we get into the Objects like (RulerObject, TerrianObject, MeshObject, ImageObject, GridlinesObject, etc.)
Example: TerrianObject is class TerrianObject : public ObjectWithScreenLocation(TerrianScreenLocation)
TerrianScreenLocation inherits from BoxedScreenLocation so we would need to call functions for both those classes to get all the screen location attributes as well as the common screen location attributes function.

Candy Cane is a ThreePointScreenLocation so that one should work. I'll see if I can try it out since you said that was failing.

@cybercop23
Copy link
Collaborator Author

Hey... okay.. I just added the 2Point that I needed for CB (channel block) & SL (SingleLine) and also added the DimmingCurve part.
I was just going to ask... and you already answered, so I'll combine 2&3 and just pass a parameter (I wanted to do that, but didn't know how you felt about combining functions like that).

CC works fine now with 3Point. Yes, I have an issue with Matrix, althought Tree work, so I'll dig more into it.

What you can take a look at for me is importing with submodels. If the import has submodels, althought all 10000% correct, xlights freezes. Kept trying to pinpoint but nand. Again, if I combine all xmodel -> rgbeffects, ALL loads correctly, even SM.

@AzGilrock
Copy link
Collaborator

AzGilrock commented May 12, 2024

So for the CustomModel compression I was thinking we could compress it even more. 90% of the models are continuous numbering so you really only need the row and column for each node. So have an initial field that indicates compression type. Maybe leave a couple extra fields reserved if we want to have additional sub-types.

Type 1: (use this type for custom models that have continuous numbering with no repeats and no gaps)
[compression type], [node count], [reserved1], [reserved2], row1, col1, row2, col2, row3, col3, etc.

Type 2: (use this type for custom models that have repeats or gaps). <- this is how you have it + the header
[compression type], [node count], [reserved1], [reserved2], row1, col1, [node value], row2, col2, [node value], etc.

@AzGilrock
Copy link
Collaborator

Darn it all the placeholder symbols are making it show up blank let me edit that comment above.

@cybercop23
Copy link
Collaborator Author

cybercop23 commented May 12, 2024

| -> Type 1: (use this type for custom models that have continuous numbering with no repeats and no gaps)
[compression type], [node count], [reserved1], [reserved2], row1, col1, row2, col2, row3, col3, etc.

Hmmm... did you mean r1,c1,r2,c1,r3,c1
What'a an example for this? With Matrix, I've added individual start nodes on Circle and Star (PR done) and will Do it on Matrix also, thus no need to turn a matrix into a CM.

@AzGilrock
Copy link
Collaborator

You find node 1 and grab its row/col, find node 2 add its row/col, etc.

So this model below would be
1, 3, 0, 0, 2, 1, 4, 2, 1, 3

So that's type 1, 3 nodes, node 1 at row 2 col 1, node 2 at row 4 col 2, node 3 at row 1 col 3.

Screenshot 2024-05-12 at 2 10 56 PM

@cybercop23
Copy link
Collaborator Author

Okay.. done... How do we choose between type 1 and type 2? Look at Node count and HxW and if all filled we go with 1? Or since I have both.. very small precessing... I can always pick the smallest string length.

RG CM=289,616, CM2.0.1=10,776, CM2.0.2=16,629

image

@AzGilrock
Copy link
Collaborator

Well you can only use Type 1 when the node numbers are consecutive

@AzGilrock
Copy link
Collaborator

There's going to be some models that use the same node number multiple times like 8 channel singing face models so they would need to be type 2. So maybe a function that checks for duplicate numbers or gaps

@cybercop23
Copy link
Collaborator Author

There's going to be some models that use the same node number multiple times like 8 channel singing face models so they would need to be type 2. So maybe a function that checks for duplicate numbers or gaps

KK.. CM has IsAllNodesUnique() so I can use that to check

@cybercop23
Copy link
Collaborator Author

image

@dowdybrown
Copy link
Contributor

By changing the CustomModel field, you are essentially changing the public API for the models that get posted on prop vendor websites. If you move forward with this, then I would request that you make the type field negative values (-1, -2). That way, if the CustomModel field starts with a "-", it could be interpreted with the new logic, and if not, the old logic could be used.

@cybercop23
Copy link
Collaborator Author

By changing the CustomModel field, you are essentially changing the public API for the models that get posted on prop vendor websites. If you move forward with this, then I would request that you make the type field negative values (-1, -2). That way, if the CustomModel field starts with a "-", it could be interpreted with the new logic, and if not, the old logic could be used.

There will be support to automatically convert from the current format to the new one, once the layout is saved. Import will still take the existing format, but once saved in the current show folder, it will become the new format.

@AzGilrock
Copy link
Collaborator

AzGilrock commented May 13, 2024

By changing the CustomModel field, you are essentially changing the public API for the models that get posted on prop vendor websites. If you move forward with this, then I would request that you make the type field negative values (-1, -2). That way, if the CustomModel field starts with a "-", it could be interpreted with the new logic, and if not, the old logic could be used.

Anything in the new format has a different XML signature and the new function in XmlSerializer detects the format. The new ones have the [models type="exported"]. I don't think anything that parses the old XML will even work parsing the new ones because its one more level down than the old one so that it can match how rgbeffects is constructed.

@AzGilrock
Copy link
Collaborator

I didn't like how the model signature for model export did not match how it was saved in rgbeffects and the goal of the new XmlSerializer is to be able to work for both functions.

@dowdybrown
Copy link
Contributor

I didn't like how the model signature for model export did not match how it was saved in rgbeffects and the goal of the new XmlSerializer is to be able to work for both functions.

Understood. So anyone else reading vendor-posted model files will need to make similar changes. And in the short-term, we can assume there will be files posted in both the old and new formats, so third parties will need to support both.

@AzGilrock
Copy link
Collaborator

I know there is a model version number we store with the model that is used to know when it was created. We should probably roll that number along with this PR and then that version number tag can be used to know which parsing method to use.

@AzGilrock
Copy link
Collaborator

Found it.....in ModelScreenLocation.h we should update CUR_MODEL_POS_VER to "8" and then every model saved will have the versionNumber=8 tag. That way we will know all the models that were saved using this new XmlSerializer code.

@cybercop23
Copy link
Collaborator Author

Well.... well... well.... did not expect this.... mindblown
image

I have a version that can now import the new "compressed" format. I'm working on supporting depth, but anything with type 1 or 2 & single depth works. Those are the results above.
For depth... since who know what they heck they are doing, I'll treat it as type 2.

@AzGilrock
Copy link
Collaborator

So a huge speed increase on the loading times? I'm also expecting a nice increase in overall program speed once we can eliminate all the internal ModelXML stuff cause its got to take a lot of time to constantly be processing XML strings.

@cybercop23
Copy link
Collaborator Author

So a huge speed increase on the loading times? I'm also expecting a nice increase in overall program speed once we can eliminate all the internal ModelXML stuff cause its got to take a lot of time to constantly be processing XML strings.

Not only load times, but also reinit on any layout tab change.... been spending lots of time to optimize whatever code I'm adding, but this is just crazy...

@cybercop23
Copy link
Collaborator Author

@AzGilrock any suggestions/ideas if/how to change the data that's showing up in the Model Data grid? Looks like it wants the csv format to populate the notebook page, so I'd have to expand it for that function, or maybe/somehow use a different process...

std::string CustomModel::GetCustomData() const { return ModelXml->GetAttribute("CustomModel").ToStdString(); }
...
std::string data = m->GetCustomData(); _saveModelData = data;
....
UpdatePreview(_saveWidth, _saveHeight, _saveDepth, _saveModelData);

@AzGilrock
Copy link
Collaborator

AzGilrock commented May 14, 2024

I'd need more time to digest everything that CustomModelDialog class is doing. It started driving me nuts looking at the Setup function which appears to grab the custom data string, writes it out into the grid, then makes a call to UpdatePreview which appears to call GetModelData which pulls the values back off the grid and then writes them back into the XML. Unless I didn't follow things right it seems really inefficient.

@cybercop23 cybercop23 marked this pull request as ready for review May 15, 2024 12:57
@cybercop23 cybercop23 marked this pull request as draft May 18, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants