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

Add remove text3d #2088

Merged
merged 3 commits into from
Nov 26, 2017
Merged

Conversation

SergioRAgostinho
Copy link
Member

This supercedes and closes #2062, and fixes #2049.

I think you can do whatever you want with the text3D in terms of adding and removing (except for adding items with ids finishing in *). I tried a number of cases with both text3D overloads.

@SergioRAgostinho SergioRAgostinho added this to the pcl-1.9.0 milestone Nov 15, 2017
@SergioRAgostinho
Copy link
Member Author

I need to fix the sign comparison warnings.

@SergioRAgostinho
Copy link
Member Author

SergioRAgostinho commented Nov 16, 2017

Explaining a little the logic behind what was done just for text3D:

  • The id added to the shape_actor global map, is a unique id composed of a "concatenation" of the id provided and the viewport number.
  • When you specify a viewport number bigger than 0, it builds that uid, checks if it exists and if not, it adds it. Same process with deletion.
  • The difference to when you specify a viewport 0, is that when that happens, the process mentioned above point above is repeated for all available viewports numbers.

"Special cases":

  • Assume you have two viewports in place, if you have an id "text" on vp 2 and then try to add "text" to all viewports, it will fail and add none.
  • Same situation as before, but now you want to remove "text" from all, it removes text from vp 2.

@taketwo taketwo self-requested a review November 16, 2017 10:09
// check all or an individual viewport for a similar id
rens_->InitTraversal ();
rens_->GetNextItem (); //discard first because it's not a renderer for the viewps
for (size_t i = std::max (viewport, 1); rens_->GetNextItem () != NULL; ++i)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand how this loop works. Could you please elaborate why initializing i with this value? Also, why breaking on line 690?

Copy link
Member Author

Choose a reason for hiding this comment

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

If viewport is 0, you're adding it to all viewport, and since they're numbered incrementally their id numbers will be {1, ..., #existing_viewports}. If viewport is bigger than 0, then you know the user is specifying the specific viewport id to add the text.

So this means that the ids you will be addressing are either from [1, #viewports] or simply the one specified in viewport, as long as its bigger then 0.

Regarding the break, you only cycle all possible ids when viewport = 0. If the user specified a single id, only that id is to be checked, that's why you break after a single iteration.

Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, now makes sense.
I did not test this, but I assume you did.

@SergioRAgostinho
Copy link
Member Author

Yeah. The case I tested had 2 viewports and I tried variants of both the Text 3D oriented and non oriented. The cases were roughly set up as following:

  • Add to 0, Delete from 1; //ok
  • Add to 0, Delete from 2; //ok
  • Add to 1, Add to 0; //warning, no addition
  • Add to 0, Add to 2; //warning, no addition
  • Add to 2, Delete from 0; //ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot removeText3D from specified viewport
2 participants