[vtkusers] vtkChartXY

[Marcus D. Hanwell]

Hi Hagen,

I was away at a meeting, and am getting caught up now...

2010/11/8 Hagen Mölle <h.moelle at googlemail.com>:
> Hi Marcus,
>
> did you have time to look at my code. It would be nice to get some feedback
> (maybe I should recode some part or there is something I forgot to
> implement).

I would love it if you uploaded the changes to Gerrit, so that I could
give you some line-by-line comments where appropriate. There are
trailing whitespaces that were introduced, these are easy to clean up.
Your code often exceeds 80 characters, we ask that you split lines
like that. Lines like,

else if ((((this->LogScale &&
!this->LogScaleReasonable)?1.0:this->TickInterval) == 0.0 ||
this->TickInterval == -1.0))

Should be split, and have space around the ? and : in the ternary.
That is quite a nested else if, and I would prefer we revise it to
make it clearer what it is intended to do. There are also lines such
as,

// If the values min and max are not within limits we set defaults
+  if (min < 1.0) min = 1.0;
+  if (min > 9.0) min = 1.0;
+  if (max < 1.0) max = 9.0;
+  if (min > 9.0) max = 9.0;

They violate the VTK code style guidelines, it must be,

if (min < 1.0)
  {
  min = 1.0;
  }

I assume the final line should have been max rather than min in that
block? It seems like they are mutually exclusive and an else if would
be more appropriate for the two pairs (if min is less then 1 it cannot
also be greater than 9. There are C style casts, where we require C++
casts (static_cast, etc).

I see you are taking bool and int values by reference. To add this we
would also need to add a test, which would be great for me to see the
new code in action as well as ensure it continues to work. I would
like to continue working on this, and get it into the charts - it has
been on my todo list for quite some time and it looks like you have
made a great start.

http://www.vtk.org/Wiki/images/8/81/GitVTKCheatSheet.pdf

The above shows you some of the steps to get this into a topic branch,
and uploaded to Gerrit. I could provide more specific feedback there,
and even make additional commits to fix up some of these issues. It
will also ensure it stays in my list of things to do. Thanks for
working on this, I look forward to adding this to the charts - I think
it will be a well received feature addition.

Thanks,

Marcus

>> 2010/10/26 Hagen Mölle<h.moelle at googlemail.com>:
>>>
>>> Hi Marcus and Eric,
>>>
>>> I am finished with writing a new logarithmic scale for logarithmic axis.
>>> I
>>> produced a patch too (git diff>  VTK.patch). Is there a specific
>>> procedure
>>> to merge the code into current master of VTK. Before doing this you
>>> hopefully want to look over the code and test it.
>>>
>>> Some remarks to my changes:
>>>
>>> 1. If axis labels are large then they will overlap.
>>> 2. If order of magnitude of the axis is lower than/equal 5 then labels
>>> will
>>> be ... 1 2 5 10 20 50 100 ... Other wise labels will be ... 1 10 100 1000
>>> ...
>>
>> I don't see any attachment. I am in the process of writing something
>> up on using it, but Gerrit will be a preferred method of contributing
>> changes to VTK. You can push a topic branch, I can easily fetch it
>> into my local tree, test it and comment on it. For now, I would
>> suggest being very brave and testing it out, using 'git format-patch'
>> to generate a patch file, or pushing a branch on one of the services
>> such as Gitorious.
>>
>> Marcus
>> --
>> Marcus D. Hanwell, Ph.D.
>> R&D Engineer, Kitware Inc.
>> (518) 881-4937
>
>