TrueBend: re-defined method for #pick_segment(points)?

Developers Ruby API
1

I think I have a problem with the re-defined method in row 41:

Originally here:

#pick_segment (points)

method is defined by the same name.

I’m using in my plugin the code somthing like this:

module Mymodule
  class Tool_test_picksegment
    ###dirty!!###
    def initialize

    end
    def onMouseMove(flags, x, y, view)
        ph = view.pick_helper
        ph.init(x, y, 10)
        pick_add = ph.pick_segment([ORIGIN,[1,1,1]])
        puts pick_add
    end
  end
end
Sketchup.active_model.select_tool(Mymodule::Tool_test_picksegment.new)

If the TrueBend has not been used befor, the code is doing what it shoud do. But after TrueBend has been used I got the following error:

Error: #<NoMethodError: undefined method `*’ for nil:NilClass> C:/Users/dezmo/AppData/Roaming/SketchUp/SketchUp 2015/SketchUp/Plugins/tt_truebend/dpi/pick_helper.rb:43:in ‘pick_segment’
main :9:in ‘onMouseMove’

It seems to me that the method defined by @tt_su somehow is “global” or “visible from outside”, and coming before the “original one”, then give an unexpected result.
Sorry about my “no professional description” but I’m still not familiar in deep with all Ruby magic.

I think I heve been properly calling the #pick_segment() method from “isolated” namespace above, but I have no real clue about the all the code of ThomThom, specially the:
super(*args)
I guess this causing the “problem” and make me confused. :confused:

I would appreciate it if you would give an explanation and tell me what’s going on?
How can I call the “original” #.pick_segment(segment) method??

Did I made a mistake with my test code above or could it be something else… ? :worried:

(BTW TrueBend can be download from:
Extension | SketchUp Extension Warehouse )

2

Go back to the API pick_helper methods…
Class: Sketchup::PickHelper — SketchUp Ruby API Documentation

3

Sorry TIG, but I don’t get you.
The API method, I’ve been already qouted that in my post above. And I’m using that as described.
My problem is WHEN the TrueBend installed it is **overwritning the original #pick_segment method for pick_helper.

Please copy-paste my code above to console. It will work.
Then: Use TrueBend. (just bend some random group)
Then Please copy-paste my code above to console again. And check. It will fail.

4

To me it seems these are two unrelated issues…

No, it defines #pick_segment in the module HighDpiPickHelper with which thomthom extends the single Sketchup::PickHelper instance that is only used within his own View class. If I read the source code correctly, it should not affect other extensions (and apart from that, extending core classes is against EWH requirements, so especially a SketchUp employee would not give a bad example by doing that).

So what is the problem? Do you suspect it causes an error in your extension?

The error that you mention occurs in TrueBend. It says that args[1] was nil and thus does not have a * method. This looks like the first signature #pick_segment(points) ⇒ Integer, false

1 Like
5

The error occurs because WHEN AN ARRAY IS PASSED AS THE 1st ARGUMENT to the Truebend #pick_segment() method, it IGNORES that 1st argument as an Point3d array (each element with the normal 3 elements for coordinates or an actual Geom::Point3d object,) and then EXPECTS at least 2 more arguments for x and y scale factor.

Dezmo only passed an array with 2 points.

Thomas wrote his method to REQUIRE x and y scale factor, and allow the aperture argument to be optional.

@thomthom I see other potential failures for your Truebend method. The “super method” allows the points to be passed as a list of normal parameters for both overrides, but your wrapper method cannot handle points passed this way. In fact it may cause a problem when no point is passed as your else clause is treating the 1st argument as an x factor when the “super method” (as the docs imply,) requires at least one point-like object as the first parameter.

1 Like
6

My full respect for ThomThom and his work. It’s far from me to suspect him. :wink:
But there is definitely a conflict between my plugins and TrueBend.
I would like to clarify, what is the problem with my code. If there is an issue, I have to “recall” my plugin from sketchUcation and modify it…
I created a "dirty " snippets in my first post to reproducible demonstrate the problem. I have been tested on SU2015 and SU2019 by execute it via console. Same result. If the TrueBend has been used before my dirty code executed, the issue is appearing…

I understood, why the error occurs, if it is passed to Truebend #pick_segment() method.
My questions are now:

  1. If it is passed to Truebend #pick_segment() method, means TrueBend has been extended one of the core classes. Am I right?
  2. This extended class can not handle the “original” : #pick_segment (points) ⇒ Integer, false . Does it means that the implementation of extending core class is not “perfect”?
  3. Do you think my "dirty " snippets above in principle right? Or do I need to modify? Or the TrueBend code needs to be modified?

Thanks a lot!

7

Could the problem be that TT extends the view object, thinking it is transient, but then the API returns the same object to another extension too?

Edit: Here ThomThom extends the PickHelper returned by View#pick_helper. It appears he thought this was a transient object, while the same object is actually returned by that method in other plugins too.

This would also explain why you have to have used TrueBend, not just had it installed, for the error to occur.

1 Like
8

I’ve created an issue in the TrueBend repo.

Also it appears you are using wrong arguments for pick_segment. If you change ph.pick_segment([ORIGIN,[1,1,1]]) into ph.pick_segment(ORIGIN, 1,1,1), it may work correctly, even if TrueBend’s method is wrongly shadowing the native method.

1 Like
9

Thanks a lot for that! ( Now I can sleep calmly … :wink: )

In my real plugin I ‘m passing a points (Array’<'Geom::Point3d>) without x,y (screen mouse position in pixels) and aperture as described in an example and the first overloads here:

Your suggested workaround is passing only one Point3d which is not enough… :wink:
But I’ll try to use #pick_segment (points, x, y, aperture = 0) at the evening. Thanks for suggestion!

10

Core object, not core class. In Ruby, you can add/redefine methods of instances and it has only impact on this very instance, not the class.

The possible issue that Christina points out is that apparently not only core classes are shared between extensions, but also instances. Since the API is a thin layer on top of SketchUp’s C code, me and most other developers would have thought that calling an API method returns a fresh reference to an object that only your extension will be using and other extensions receive a new object. This is not the case for parts of the API that are implemented in plain Ruby (LanguageHandler) or that receive objects from Ruby (Sketchup.register_extension once allowed registering arbitrary objects). Maybe the API has a caching mechanism either on the Ruby side (same View reference passed to multiple extension tools) or on the C side (cached C object passed to multiple extensions is always resolved to the same modifiable Ruby reference).

2 Likes
11

Oh, it seems TT’s code doesn’t support that first overload.

Btw, if you write Geom::Point3d.new(1, 1, 1) rather than [1, 1, 1] it is much easier to understand what you mean.

2 Likes
12

And another argument for explicit Point3d:
While such syntactic sugar makes it easier (lazier) to call methods, it makes it harder to correctly implement methods. (A method accepts either point or array of points; an array is passed; is the array a point or an array of points?)

It’s best to avoid such alternative method calls to reduce complexity and minimize possible mistakes.

2 Likes
13

You and @eneroth3 are absolutely right I’m will try to override my laziness… :blush:
Thanks for detailed answers and explanations!!

1 Like
14

Oh man! I’m really sorry about this! I wasn’t aware that PickHelper objects were reused. Christina and Andreas nail it. The Sketchup::PickHelper is cached by SketchUp. A source code comments explains this is done because the internal pick helper object is “too big” to let Ruby’s garbage collector cleans it up. So this is a memory optimisation. There is only one PickHelper object per view - which for all intents and purposes in SketchUp means one per model.

Implications of this is that we should not extend Sketchup::PickHelper like I have done. And… also you cannot rely on it for storing long term state - as any other usage of a pickhelper would change the state of the view global pick helper.

I’m logging an issue to update the SketchUp API documentation: Document that Sketchup::PickHelper instances are reused · Issue #198 · SketchUp/api-issue-tracker · GitHub

And I’ll fix my extensions that use this pattern ASAP. I think it’s TrueBend, Vertex Tools and SUbD.

Again, I’m really sorry for this mess. I guess I should have suspected it since you have to use view.pick_helper to obtain an instance of Sketchup::PickHelper.

2 Likes
15

Even the best of us can make mistakes! :slight_smile:

16

That’s okay! Those who work, can make mistakes! :slight_smile:
:+1:

17

When I wrote the “sub-class” I wrote it specifically for TrueBend and how it used the PickHelper - so I didn’t bother too hard about mapping the signature 100% to the original. The original set of overloads is awkward - allowing the first argument to be an array of points, or all arguments could also be points.

The signatures of SUs implementation is kind of like this:

def pick_segment(points)
def pick_segment(points, x, y)
def pick_segment(*points)
def pick_segment(*points, x, y)

As Andreas mention, the syntactic sugar of supporting def pick_segment(points) and def pick_segment(*points) complicates the code - especially when you have additional arguments. Allowing that pattern before other arguments in a signature is really awkward.

IMO it would be better to have just:

def pick_segment(points)
def pick_segment(points, x, y)

Simpler implementation, clearer API. Alas…

2 Likes
18

I see the problem not in your code, but in the API. Other developers could have done the same. We all learned something.

It is still surprising in comparison to other API methods we are used to (but an understandable optimization). Apart from documenting, do you think there is something that can be done in the API to prevent it (not everyone will have read every line of the docs, me included)?

1 Like
19

Could potentially overwrite the extend method on the Sketchup::PickHelper class et al. (entities etc). But I’m not sure if I want to be as harsh as raising an error. While this is a go-go in production code, I’ve monkey-patched Ruby API and SketchUp in the past for debugging purposes - as an extension developer and as a SketchUp developer. But maybe adding a warn message. We do have warning already for not closing Ruby operations - but those needs to be enabled with Sketchup.debug_mode = true. Could also add warnings behind that flag - but I doubt many devs would then see the warnings.

20

Ok, I have a fix for TrueBend ready. Wasn’t many lines really:

I’ll get that uploaded to EW and SketchUcation. I’ll then do the same for Vertex Tools.

Too bad the old versions will probably float around for a while… :frowning:

1 Like