Review Request 119621: Support for Geogebra's intersections and constrained points adn Loci in kig ( GSoC 14 )

[David Narváez]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119621/#review64180
-----------------------------------------------------------


I had a quick look at the changes and there are indeed lots of things to consider here, but I listed out some low hanging fruits that can be sorted out first, so please take care of those and upload a new version of the patch. You may also want to take care of formatting the sources in your next version of the patch since I see some lines that start with tab indentation.


geogebra/geogebra.xsl
<https://git.reviewboard.kde.org/r/119621/#comment44819>

    var1 is not a descriptive name



geogebra/geogebra.xsl
<https://git.reviewboard.kde.org/r/119621/#comment44820>

    This test should go in regardless of the fate of this patch (once the variable name is fixed), so go ahead and add this test as a separate commit.



geogebra/geogebra.xsl
<https://git.reviewboard.kde.org/r/119621/#comment44818>

    Break this into its own commit - no need to review it



geogebra/geogebratransformer.h
<https://git.reviewboard.kde.org/r/119621/#comment44821>

    No commented code in the patches. If this is work in progress or you are not sure it will go in, stash it in your local git repository.



geogebra/geogebratransformer.cpp
<https://git.reviewboard.kde.org/r/119621/#comment44822>

    No commented code in the patches.



geogebra/geogebratransformer.cpp
<https://git.reviewboard.kde.org/r/119621/#comment44823>

    No commented code in the patches.


- David Narváez


On Aug. 10, 2014, 9:46 a.m., Aniket Anvit wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119621/
> -----------------------------------------------------------
> 
> (Updated Aug. 10, 2014, 9:46 a.m.)
> 
> 
> Review request for KDE Edu and David Narváez.
> 
> 
> Repository: kig
> 
> 
> Description
> -------
> 
> Line-Line intersection is perfect and simple to implement with the existing model. Other Intersections are a bit challenging since they output more than one object (point) unlike the other commands ( which output just one object).
> LineConic, Circle-Circle intersections seem fine to me but I have some trouble in understanding whether my approach is correctly assigning the output-label to the obtained intersection points ( while filling the m_objectMap). Please check whether the IntImp parameter I am passing to the IntersectionTypes is correct or not. ConicConic Intersections produce 4-points sometimes and they have been implemented as Line-Conic intersections in Kig. So, we have to find the Radical-Lines of the conics first before finding the intersection-points. Currently , there is an issue that the Radical-lines are also added to the document ( while they should not be ), but this should be simple to take care (If you give the go-ahead to this approach). 
> 
> Constrained points are must for Locus. I have implemented them , but the extra-handling required is making things a little ugly.
> Locus is under progress...
> 
> 
> Diffs
> -----
> 
>   geogebra/geogebra.xsl 4150e90 
>   geogebra/geogebratransformer.h 5f36827 
>   geogebra/geogebratransformer.cpp aee8669 
> 
> Diff: https://git.reviewboard.kde.org/r/119621/diff/
> 
> 
> Testing
> -------
> 
> I ran some basic tests for all the intersection cases which have been implemented. Seem ok. Only issue is with the conic-conic intersection case where we get the additional ConicRadical Lines which should be visible in Kig. However, this should be easy to take care of if this implementation survives.
> 
> 
> File Attachments
> ----------------
> 
> locus_working_1
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/08/10/9559b469-3c24-41d8-b9ba-809e68f86fbe__locus_working_1.ggb
> locus_working_2
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/08/10/fce5bd47-794b-4ae3-aa21-4cd7c688c6fe__locus_working_2.ggb
> locus_working_3
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/08/10/8cc09d1b-ff42-4a99-b2ad-7d7b403c92ad__locus_working_3.ggb
> locus_crashing
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/08/10/c5c142eb-9542-4c2b-b6f8-0250ccdfb3fd__locus_crashing_parabola.ggb
> 
> 
> Thanks,
> 
> Aniket Anvit
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20140810/a86b7921/attachment.html>