Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(14864)

Issue 67168: Adding a text 3D module (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years, 10 months ago by Gael.Varoquaux
Modified:
14 years, 9 months ago
Reviewers:
prabhu
Base URL:
https://svn.enthought.com/svn/enthought/Mayavi/trunk/
Visibility:
Public.

Description

This adds a text 3D module.

Patch Set 1 #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+182 lines, -0 lines) Patch
enthought/mayavi/modules/api.py View 1 chunk +1 line, -0 lines 0 comments Download
enthought/mayavi/modules/metadata.py View 2 chunks +13 lines, -0 lines 1 comment Download
enthought/mayavi/modules/text3d.py View 1 chunk +168 lines, -0 lines 7 comments Download

Messages

Total messages: 3
Gael.Varoquaux
14 years, 10 months ago (2009-06-09 22:32:43 UTC) #1
Gael.Varoquaux
14 years, 10 months ago (2009-06-09 22:33:22 UTC) #2
prabhu
14 years, 10 months ago (2009-06-13 12:06:48 UTC) #3
Gael, thanks for the patch and placing it for review here.  I would appreciate a
unit test for this code and also fixing all issues. mentioned.

http://codereview.appspot.com/67168/diff/1/4
File enthought/mayavi/modules/metadata.py (right):

http://codereview.appspot.com/67168/diff/1/4#newcode256
Line 256: desc   = "Displays user specified text in the scene",
Perhaps something different from the TextModule would be more helpful to a new
user.  Say "Displays text at a 3D location on screen."

http://codereview.appspot.com/67168/diff/1/3
File enthought/mayavi/modules/text3d.py (right):

http://codereview.appspot.com/67168/diff/1/3#newcode1
Line 1: """ This module allows the user to place 3D text in the scene.
I would like a more detailed docstring which explains for instance why this is
different from Text and where it could be used.

http://codereview.appspot.com/67168/diff/1/3#newcode34
Line 34: text = Str('Text', desc='the text to be displayed')
It would be good to use enter_set=True. auto_set=False for all of these that use
an entry widget.

http://codereview.appspot.com/67168/diff/1/3#newcode37
Line 37: position = CArray(value=(0., 0., 0.),
For some reason the auto_set and enter_set do not work for Array/Carray.  BTW,
why are you using CArray instead of Array?  I don't see a significant difference
but usually veer away from using C<Trait>.  If the enter_set/auto_set doesn't
work I think a bug ought to be filed on traits UI.

http://codereview.appspot.com/67168/diff/1/3#newcode122
Line 122: # XXX: Terribly hackish
This is not correct.  I had replied to you on the list telling you what to do.
Look at the outline module to see how this is done.  The trick is simple, set
self.outuputs = [self.vector_text.output] and then set new.inputs = [self].

http://codereview.appspot.com/67168/diff/1/3#newcode123
Line 123: new.inputs = [Component(outputs=[self.vector_text.get_output()])]
Why are you using vector_text.get_output()!??

http://codereview.appspot.com/67168/diff/1/3#newcode129
Line 129: new.on_trait_change(self.render)
Why is all this wiring needed?  The actor is a component and will do what is
necessary when it is setup, it has its own render so it isn't clear why you are
wiring up additional ones. All you need to do is set the new.actor suitably.

http://codereview.appspot.com/67168/diff/1/3#newcode146
Line 146: """
I don't have time to figure out a solution but I do not like this solution too
much. The problem is each time you change orient_to_camera you create a new
actor and follower instead of reusing an existing one. A truly clean solution
would be to add follower support in the actor component but I don't have the
time for that and that must be done very carefully.  One thing I haven't tested
is if this will persist correctly when you turn the orient_to_camera to off.  I
think a simple unit test would also be very handy but might be a pain since
there isn't a camera.  It is still well worth trying and will help catch many
bugs.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b