SUTextSetPoint functionality "messes up model" (at least *one* way of using it)

[jonnyns]

This documents a problem I found during some recent late night code development:

SUTextSetPoint can use three variants of leader types:
SUTextLeaderType_None
SUTextLeaderType_ViewBased
SUTextLeaderType_PushPin

In the following I assume that SUTextLeaderType_None is also a legal type, i.e. I assume that it is allowed to specify this value when using the SUTextSetPoint functionality.

SUTextSetPoint can be used in three ways: point, path and point/path. This report is specificly dealing with the path variant (where one connects to an entity).

The example code below will cause the problem, such that when opening the saved file, SketchUp will report "Something minor is messed up in your model..."
The Validity Checker says:
"Entity CText shoud be erased - done"

In honesty, I cannot say that I do trust the last part of message "done", because a subsequent SketchUp application exit gives a full-featured Bug Splat.

For the record I use:
SDK_WIN_x64_2020-0-363
SketchUp Pro 20.0.373
However, I believe this issue has been present for some time...

        #define M_TO_MM(v) (unitConv * v)

        std::string outfilename = { "test_ccode_text.skp" };
	SUResult su_res;
	size_t count;

	double unitConv = (1000. / 25.4); // Dimensions in millimeter

	SUInitialize();
	SUModelRef model = SU_INVALID;
	su_res = SUModelCreate(&model);
	SUEntitiesRef model_entities = SU_INVALID;
	su_res = SUModelGetEntities(model, &model_entities);

	SUFontRef myFont;

	su_res = SUModelGetNumFonts(model, &count);
	if ((su_res == SU_ERROR_NONE) && (count > 0))
	{
		size_t n;
		SUFontRef* pFonts = (SUFontRef*)malloc(count * sizeof(SUFontRef));
		if (pFonts != NULL)
		{
			su_res = SUModelGetFonts(model, count, pFonts, &n);
			if ((su_res == SU_ERROR_NONE) && (count == n))
			{
				for (n = 0; n < count; n++)
				{
					size_t i, j;
					char s[64];
					SUStringRef su_string = SU_INVALID;

					su_res = SUStringCreate(&su_string);
					if (su_res == SU_ERROR_NONE)
					{
						su_res = SUFontGetFaceName(*(pFonts + n), &su_string);
						su_res = SUStringGetUTF8Length(su_string, &i);
						su_res = SUStringGetUTF8(su_string, i, s, &j);

						su_res = SUStringRelease(&su_string);
					}
				}

				// Use the first font found
				myFont = *(pFonts + 0);
			}
		}

		free(pFonts);	// FIXME
	}

	// Guidepoint (will be used as path)
	SUPoint3D su_pt_1 = { M_TO_MM(2.0), M_TO_MM(2.0), M_TO_MM(2.0) };
	SUGuidePointRef su_gp_1 = SU_INVALID;
	su_res = SUGuidePointCreate(&su_gp_1, &su_pt_1);
	su_res = SUEntitiesAddGuidePoints(model_entities, 1, &su_gp_1);

	// Text to path
	SUTextRef su_txt_1 = SU_INVALID;
	su_res = SUTextCreate(&su_txt_1);
	su_res = SUTextSetFont(su_txt_1, myFont);
	su_res = SUTextSetString(su_txt_1, "Txt at path");
//	su_res = SUTextSetLeaderType(su_txt_1, SUTextLeaderType_PushPin);
	su_res = SUTextSetLeaderType(su_txt_1, SUTextLeaderType_None);
	su_res = SUTextSetArrowType(su_txt_1, SUArrowClosed);
	SUVector3D su_vec_1 = { M_TO_MM(0.0), M_TO_MM(0.0), M_TO_MM(1.0) };
	su_res = SUTextSetLeaderVector(su_txt_1, &su_vec_1);
	SUColor su_color_1 = { 0, 255, 0, 255 };
	su_res = SUTextSetColor(su_txt_1, &su_color_1);
	SUInstancePathRef su_path_1 = SU_INVALID;
	su_res = SUInstancePathCreate(&su_path_1);
	su_res = SUInstancePathSetLeaf(su_path_1, SUGuidePointToEntity(su_gp_1));
	su_res = SUTextSetPoint(su_txt_1, NULL, su_path_1);
	su_res = SUEntitiesAddTexts(model_entities, 1, &su_txt_1);

	// Save
	if (!SUIsInvalid(model)) {
		if (outfilename.length() > 0) {
			su_res = SUModelSaveToFile(model, outfilename.c_str());
		}

		su_res = SUModelRelease(&model);
		SUSetInvalid(model);
	}

	SUTerminate();

[DanRathbun]

NOTE:
To save reading time, a Breakdown of the Issues found has been listed in post 9.
;)

---

In the following I assume that SUTextLeaderType_None is also a legal type, i.e. I assume that it is allowed to specify this value when using the SUTextSetPoint functionality.

It is actually the first value of an enumeration, so it's really an integer having the value 0.
So when you pass it into SUTextSetLeaderType, it will be used a switch "selector".

---

I'm not sure it was you that I said this to, but anyway, generally using the SketchUp C API, attach the object to the model (or one of it's collections) before you start setting properties on this object.

Your adding the guide point to the model entities before using it for a text leader was a good thing.

However, you are setting properties upon the text leader before it is actually a valid model entity. Ie it's still "floating" in limbo and isn't yet "owned" by the model's entities collection when you're calling property setters.

---

You are not checking the results of calling any of the geometry creation functions in your example above.

---

Does only attaching to a guide point cause an error, or how about an edge, or a face or a vertex ?

---

SUVector3D su_vec_1 = { M_TO_MM(0.0), M_TO_MM(0.0), M_TO_MM(1.0) };

It shouldn't be necessary to use converted double values to specify one of the constant orthogonal model axis.
(I can understand if your aligning something to model geometry vertices, however since the API natively uses inches, there's no need to convert the point coordinates in such cases.)

See this issue for examples of adding vector and origin point constants ...

• Predefined Geometric Helper Constants #352
  They should be in their own header file protected within a #ifndef GEOMETRY_CONSTANTS_H_ block.
  (EDIT: I added the include protection to the example.)

---

#define M_TO_MM(v) (unitConv * v)

double unitConv = (1000. / 25.4); // Dimensions in millimeter

Once again, your comments and function name is incorrect.
(1000. / 25.4) does not give a value in millimeters. It gives you a value for the number of inches in a meter.

So this leads to the function name M_TO_MM being incorrect.
The correct name (if you must have the output as part of the name) is actually: M_TO_IN.

But SketchUp uses inches internally and all values in .skp files will be in inches. So it's a bit frivolous to have a "_TO_IN" suffix on the end of every conversion function name.

[jonnyns]

It is actually the first value of an enumeration, so it's really an integer having the value 0.
So when you pass it into SUTextSetLeaderType, it will be used a switch "selector".

I know this, Dan. What I was trying to say here, is that I assume that the API designers consider it allowable to add a text path without a leader, i.e. with the enumeration value SUTextLeaderType_None. If not then the documentation should say so!! But it does not, so therefore I consider this a bug!
And since it leads to a subsequent Bug-Splat SketchUp's cleanup of the situation is far from adequate.

Secondly, for the record, the code I enter here is not production code. It is only entered for the benefit of the Trimble developers, so they can easily reproduce and debug the issue that I encounter. Thus, I believe I give them a pretty clean way of reproducing the actual issue with the least amount of work for them! I do not intend to use much effort to clean it up.
(Again, for the record, my own production code is built on top of a C++ wrapper. Giving out code on this level would hardly help, so therefore I quickly write up a runnable native code demo, to verify the issue, both to myself and for the Trimble developers at the other end).

So when you claim that there are some results that I am not checking in the demo code, then, that is true. But make no mistake about it, when I single-step through the code in the debugger - prior to publishing the code on the API Issue Tracker - I pay close attention to the result values.

Does only attaching to a guide point cause an error, or how about an edge, or a face or a vertex ?

I could, of course, try out a lot of parameter permutations here, but, quite frankly I do not like to use too much time on doing so.
I do not have access to the underlying API code, but the Trible developers do, and I trust that they are curious enough to see consequences for other parameter combinations.

Please note that the su_vec_1 vector actually gives both direction and length of the leader. In my crude demo code I was just verifying this. Again, this is not production code, just some lines of demonstration code to document erratic behavior, nothing more. And lastly, my conversion factor does just exactly what I intended it to do - in this context - make the resulting value appear in millimeters. On my computer screen a value of 1.0 shows in the SketchUp window as 1000 millimeters. That was all I intended it to do!

Now, to something more interesting:
If I do not misunderstand you (sorry if I do), you claim that one should attach an object to the model before setting parameters. In my crude demo code, I do - as you observe - first create a the text object in memory, then set its parameter values and then attach it to the model.
In many cases, I do (and have done) this, and I have never seen a problem with this.
When I attach (say) a text object, why would not its parameters be accepted together with the object (they appear to be)? Why does it matter that it is not yet "owned"?

All this said, rest assured that I read all your comments with interest. I just wonder whether this is the right place for such discussions, I thought this place was meant to enter "bug" issues. May be I am wrong?

[DanRathbun]

@jonnyns : What I was trying to say here, is that I assume that the API designers consider it allowable to add a text path without a leader, i.e. with the enumeration value SUTextLeaderType_None. If not then the documentation should say so!! But it does not, so therefore I consider this a bug!

Oh now I gotcha! No, this should not be a bug. Using SUTextLeaderType_None should be the compliment of manually setting Leader to Hidden in the Entity Info inspector when a Leader Text object is selected in the application.
(But more on this below.)

EDIT: Okay, I just tested in the GUI using the Ruby API. ONLY the "ViewBased" (1) and "PushPin" (2) types apply to Text objects connected to entities by a leader.
The "Hidden" type seen in the GUI, is actually a separate flag that determines whether the leader is shown or not. The C API does not yet have a function to set this flag.

As mentioned below, SUTextLeaderType_None is for screen notes that are attached to the model's view using x and y percentage from the upper left corner of the viewport.

So THIS report (#436) is not a bug at all. It is the result of not using the API correctly. (Likely caused by poor documentation of the SUTextRef object and some of it's methods.)
Ie, the SUEntitiesAddTexts() function does not document any error codes for invalid text object in the passed array (... it only tests if the array is NULL.)

My last comment on this is, ... would checking the validity of the SUTextRef object before attempting to add it to any entities collection have caught the misuse of the leader type ?
Hard to do as this class doesn't appear to have a dedicated validity checking function like some of the other classes do.

---

@jonnyns : ... my conversion factor does just exactly what I intended it to do - in this context - make the resulting value appear in millimeters. On my computer screen a value of 1.0 shows in the SketchUp window as 1000 millimeters.

The C API works upon files in which all length values and coordinates are in inches. Later when a user opens the file in SketchUp they can set the display units to whatever they wish, independent of the internal inch values.

Although it's not that big of a deal, what I'm saying is that your C side conversion naming is too closely coupled to a particular end user unit display setting.

@DanRathbun: Does only attaching to a guide point cause an error, or how about an edge, or a face or a vertex ?

@jonnyns : I could, of course, try out a lot of parameter permutations here, but, quite frankly I do not like to use too much time on doing so.

It was a rhetorical question not necessarily aimed at you. Ie, an open question about this issue is ... "Do we see it only with regard to guide points ?" ... "Is there something unique about guide points that causes bad text leaders ?"

So one way would be to do the same test on other objects ... especially geometry objects that have associated vertex objects.

---

Another Rhetorical Question for the issue: Does the invalid CText and BugSplat occur when the leader type is other than SUTextLeaderType_None ?

Ie, there is another kind of Text entity called a "Screen Note" or "View based Text". In the Ruby API, it is created via Sketchup::Model#add_note and uses x and y percentages of the model view window. But the C API does not have a parity method.

EDIT: Instead the C API does rely upon the type of leader as set and concludes that any Text object with leader type set to none is a screen note, and therefore should have it's position set via the
SUTextSetScreenPosition method.

The C API implementation of SUTextRef lacks parity with the Ruby API as the "Hidden" flag does not yet have a C API function. (Although it appears in the leader type dropdown in the GUI, it is not a type, it's a boolean flag.)

---

Perhaps the issue is actually caused by the instance path functionality, which is a relatively new exposure to both the Ruby and C APIs.

EDIT: I mused this before I realized that the OP's example is misapplying the text leader type of a View Note to a SUTextRef object that then has leader vector, arrow type and instance path set on.

In the example case, an instance path with no "root" (ie a path with only a leaf entity) is used.
I would ask in this case before using, does SUInstancePathIsValid return true ?

@jonnyns : All this said, rest assured that I read all your comments with interest. I just wonder whether this is the right place for such discussions, I thought this place was meant to enter "bug" issues. May be I am wrong?

Anyone's reports must stand up to some peer scrutiny.

If you review other reports you'll see quite a bit of discussion, and challenges to specific conclusions. In some cases the facts are challenged. In others the causes are disputed. In some cases it turns out not to be a bug, but violating the intended use of the API.

@jonnyns : If I do not misunderstand you (sorry if I do), you claim that one should attach an object to the model before setting parameters.

It is not my claim. It is the advice of the guys who wrote the SDK. Unfortunately, it is poorly documented. To read the advice, you need to have read the examples that came with the SDK download archive.

I am sorry the C API documentation is so poor. I've been logging issues for years to improve it.
Here is one major doc issue that involves the order of API calls ...
C API documentation to help explain creating component definitions
... which was instigated by a public forum query where the API was throwing an access violation.

The above was not the only recent case. There was another, but it may have been discussed in private. Anyway, ... when the order of calls were changed the issue disappeared.

@jonnyns : In my crude demo code, I do - as you observe - first create a the text object in memory, then set its parameter values and then attach it to the model.
In many cases, I do (and have done) this, and I have never seen a problem with this.

Yet ... we are seeing a problem now are we not ?

When one is debugging (or reporting) an issue, it is generally advisable not to introduce any more possible issues into the situation.

@jonnyns : When I attach (say) a text object, why would not its parameters be accepted together with the object (they appear to be)? Why does it matter that it is not yet "owned"?

It may not be all properties, ... it may just be that you are setting the leader vector before setting the leader point. The text needs the vector for it's position, and the vector direction and length are calculated from the leader point.

This is however, generally a good question for the authors of the C API.

There are some examples that show for instance, setting the texture property on a material object before attaching it to a face (and attaching to a face without first adding it to the model's materials collection.) [Bottom of main C API entry page.]

So even the examples are not consistent.

---

The bottom line, is if you feel I'm "nit pickin'" your example too much, then ignore my comments.

[DanRathbun]

@jonnyns : So when you claim that there are some results that I am not checking in the demo code, then, that is true. But make no mistake about it, when I single-step through the code in the debugger - prior to publishing the code on the API Issue Tracker - I pay close attention to the result values.

I find it distressing that after having set the leader type to none, that any of the subsequent calls to set arrow type, set leader vector, the instance path, etc., did not return SU_ERROR_INVALID_INPUT. These functions should do this if the leader type of the object does not use arrows or vector, or a path to an entity.

The core code that loads components or model files should be able to determine if any text object leader types were set incorrectly (to none,) and set them to view based (as a default) without having to delete the object.

The situation where having caught a problem object, fixed (or deleted it) and the application still BugSplats is dumb!

[jonnyns]

I did now experiment a little with the order of things. As the number of possible permutations are pretty large, I did only a few. According to what I observe nothing seems to indicate that the order matters (in this case!).
Attaching first, and setting the leader to none afterwards, still leads to Sketchup becoming instable after SketcUp claims to have fixed things. The behavior does not change in any way.

And as well, validity checking of SUTextRef before adding, does not flag any apparent problem.

As said, SUTextSetPoint can be used in three ways: point, path and point/path.
The SketchUp instability - from the use of SUTextLeaderType_None - appears to affect the latter two, not the point way.

Whether or not is it legal to apply SUTextLeaderType_None, I hope the instablity issue will be dealt with. It obviously demonstrates a problem in SketchUp's ability to rescue the situation.
And I certainly hope for an improved documentation! And, of course, more useful result codes to indicate whether the API is employed according to the intentions of the API designers!

I think I will leave this issue to the Sketchup's developers now. At least my understanding of this functionality has improved. It is sad the API documentation is not at a better level, though.

I invariably find your comment valuable, Dan. However, in this context, where my intention is solely to document a reproducible example, I will definitely exercise my right to choose which comments I react to and implement.
But beware, just because I do not implement your comments immediately (in this context), does not imply that I do not find them valuable and useful. On the contrary! So please continue to respond! I have definitely learnt a lot from
your feedback both now and in the past!

[DanRathbun]

And I certainly hope for an improved documentation! And, of course, more useful result codes to indicate whether the API is employed according to the intentions of the API designers!

We've been discussing how to better handle the difference between Screen Notes and Leader Text objects for at least 5 years.

I posted an feature request issue here as a result of the 5 years of discussion ...

Issue #207: View notes could be a subclass of Sketchup::Text

If they had their own subclass, you'd not have gotten confused so easily as to the intended use of SUTextLeaderType_None.

[DanRathbun]

@thomthom What happens if using the C API, a coder adds a View Note Text object to a group or component definition's entities ? We cannot do this with the Ruby API.

I did do it manually and saved out the file, and then imported it as a component into a new empty model. This View Note is ignored.
If I then SaveAs the component out as a new component file, and then directly open this component, the ViewNote is gone.

Anyway, what I'm getting at is that it is a disparity for the C API to make coders attach ViewNote Text objects to an entities collection. (Unless the SUEntitiesAddTexts() function is smart enough to reject View Notes for definition's and only allow them for the model's entities.)

[DanRathbun]

Breakdown of Issues

---

Core model load Issue

• The core should be able to determine if any text object leader types were set incorrectly (to none,) and set them to view based (as a default) without having to delete the object. (Ie, if no screen position was set and/or point, instance path, arrow types, or leader vector was set.)

• Having caught a problem text object, fixed (or deleted it) ... the application should not BugSplat.

---

Documentation Issues

SUTextSetLeaderType()
Doc: Make it crystal clear that SUTextLeaderType_None is ONLY for View Notes and NOT for 3D model text callouts (with or without leader displayed.)

SUTextSetPoint()
Doc: Make it crystal clear that this function is for setting a 3D point for 3D text callout objects that have leader type other than SUTextLeaderType_None (which is ONLY for View Notes.)

SUTextSetScreenPosition()
Doc: "Sets the screen position for text with no leader." Should be changed to something like:
"Sets the screen position for a view note text object. Will return an error if called upon a Text object that has a leader type other than SUTextLeaderType_None."

SUEntitiesAddTexts()
Doc: Should add a note similar to: "View Note Text objects (with type of SUTextLeaderType_None) should only be added to the model's entities collection."

---

Better Error Handling

The following functions should return an error (probably SU_ERROR_INVALID_INPUT,) if the text object has type set to SUTextLeaderType_None :

• SUTextSetArrowType()
• SUTextSetLeaderVector()
• SUTextSetPoint()

The SUTextSetLeaderType() function should return SU_ERROR_INVALID_INPUT if any of the above properties have previously been set, and this call changes the leader type to SUTextLeaderType_None.

---

Missing Functionality

bool SUTextIsValid(SUTextRef)
A new function to check if any of the properties have resulted in an invalid object. This can be used both internally by SUEntitiesAddTexts() and by 3rd party coders.

SUTextSetLeaderHidden(SUTextRef, bool)
A new function (for API parity) that sets whether a 3D text callout has it's leader hidden.
(Parity with Sketchup::Text#display_leader= from the Ruby API.)

• Jon reports below that if the leader vector is omitted or a zero length, then the leader hidden flag is set. (This is non-intuitive and not documented.)

SUTextGetLeaderIsHidden(SUTextRef, *bool)
A new function (for API parity) that gets whether a 3D text callout has it's leader hidden.
(Parity with Sketchup::Text#display_leader? from the Ruby API.)

~

[jonnyns]

@DanRathbun
This is my summary of the 4 different text "modes" available from the C SDK.

[thomthom]

wow - a lengthy thread. Thank you both for some summary to the end of this.

Right now I can only log this as diving into all the details here isn't something I have time for right now. This needs to be schedules.