fix issue for multiple attributes with same name (crashed the import) #2
Conversation
|
Hi Benjamin, I assume this should only happen on attributes with different types. Once this work done, you should also submit a PR to the official Tulip repo. PS: I can only work on that Tulip fork on my spare time and I am far to reach |
| @@ -71,6 +71,9 @@ def startElement(self, name, attrs): | |||
| self.attrLabel = 'label' | |||
|
|
|||
| # Create a Talipot property compatible with the attribute type | |||
| while self.graph.existProperty(attrName): | |||
| attrName = '_'+attrName | |||
There was a problem hiding this comment.
Please add spaces around the + operator.
You should try the flake8 tool by the way, it really helps
to write clean Python code and it is really easy to integrate it in IDE.
|
Thanks Antoine,
The fix is done, there is still the test to write though, but I can share
the dataset that triggered the error originally.
As for the repository I though I could help everyone at once, and tried to
fork also tulip, but it appears that I cannot work only on one fork.
But it seemed that my PR worked on both I guess. This is my first time
trying this on a fork of a fork, so I'm not sure what goes on here.
Anyway, and thanks for the Talipot bundle link! as long as I can I'm gonna
try to work and contribute on both.
--
Benjamin
…On Wed, 11 Dec 2019 at 19:30, Antoine Lambert ***@***.***> wrote:
Hi Benjamin,
I assume this should only happen on attributes with different types.
How about suffixing the Talipot property to create with _<type> instead ?
Also we should add some tests for the GraphML import using really simple
graphs in that format.
Once this work done, you should also submit a PR to the official Tulip
repo.
I am done on officially contributing to Tulip and prefer focusing on that
fork
as I am free to go in the direction that I want for the framework (notably
in terms of code style, API design, GUI, Python integration, rendering
engine,
...)
PS: I can only work on that Tulip fork on my spare time and I am far to
reach
a releasable version yet. Nevertheless, you should be able to test my
advancements
on this through this dmg bundle
<https://bintray.com/anlambert/talipot_binaries/download_file?file_path=Talipot-1.0.0-dev.dmg>
updated on each push on the master
branch (when the associated CI job does not timeout as it is the case
actually).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2?email_source=notifications&email_token=AANMAFJJIILS5R3KHTDLRFLQYEWVPA5CNFSM4JZOSOYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGUD4RI#issuecomment-564674117>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANMAFMJZEIRBPKZDMWYCXDQYEWVPANCNFSM4JZOSOYA>
.
|
|
Hey Benjamin, Sorry for the late answer, I did not have enough time to work on this during the last days.
Cool ! Can you squash the commits first and push force your branch before I merge this ? Also it is a better practice to create a feature branch locally on your fork and push it to GitHub in order to create a PR from it. Yes I am interested by your GraphML dataset, it will help me write proper tests (I handle it, don't worry).
Indeed this will not work as I started diverging a lot from the Tulip codebase and I changed the project name. If you want to contribute to both, you will need to have a fork of Tulip and a fork of Talipot in separate folders.
You should already benefit of some improvements on MacOS using my bundle as I removed the dependency to deprecated QOpenGL module in favor of plain QtGui and its OpenGL features: this should fix numerous rendering glitches observed on that platform. Migrating away from QtOpenGL also fixes REALLY slow selection using an Intel GPU on debian (my case) for instance. But as I said, I can not be full time on this so I have to do it incrementally to prepare the big changes Cheers ! |
| @@ -71,6 +71,9 @@ def startElement(self, name, attrs): | |||
| self.attrLabel = 'label' | |||
|
|
|||
| # Create a Talipot property compatible with the attribute type | |||
| while self.graph.existProperty(attrName): | |||
There was a problem hiding this comment.
I think a simple if should be enough here.
adding spaces around + as requested minor code cleaning now change the while into an if since we specify the type Add a test dataset for the case of same attribute multiple types.
|
Hi Antoine,
Thanks a lot for the advice, and showing me the right steps ! :)
I have to do two rebase because I could not rebase on the original commit
for some reason, but it should be fine now.
I have added the test dataset directly too, I figured out it would not be
too much of a problem since there are some other datasets to test in the
same place.
I will open a new PR.
As a sidenote, github considers that I have forked tulip already since I
forked talipot, so I would need to use a different account but, anyways it
will be ok.
But I'll have some fun with Talipot too. BTW, the layout of the python
editor is fixed and constrained in only half the window/non detachable
making it difficult to use.
I can open an issue on this if you wish.
Anyway step by step :)
--
Benjamin
…On Mon, 16 Dec 2019 at 23:36, Antoine Lambert ***@***.***> wrote:
Hey Benjamin,
Sorry for the late answer, I did not have enough time to work on this
during the last days.
The fix is done, there is still the test to write though, but I can share
the dataset that triggered the error originally.
Cool ! Can you squash the commits
<https://github.com/wprig/wprig/wiki/How-to-squash-commits> first and
push force your branch before I merge this ? Also it is a better practice
to create a feature branch locally on your fork and push it to GitHub in
order to create a PR from it.
Yes I am interested by your GraphML dataset, it will help me write proper
tests (I handle it, don't worry).
As for the repository I though I could help everyone at once, and tried to
fork also tulip, but it appears that I cannot work only on one fork.
But it seemed that my PR worked on both I guess. This is my first time
trying this on a fork of a fork, so I'm not sure what goes on here.
Indeed this will not work as I started diverging a lot from the Tulip
codebase and I changed the project name. If you want to contribute to both,
you will need to have a fork of Tulip and a fork of Talipot in separate
folders.
Anyway, and thanks for the Talipot bundle link! as long as I can I'm gonna
try to work and contribute on both.
You should already benefit of some improvements on MacOS using my bundle
as I removed the dependency to deprecated QOpenGL module in favor of plain
QtGui and its OpenGL features: this should fix numerous rendering glitches
observed on that platform. Migrating away from QtOpenGL also fixes
*REALLY* slow selection using an Intel GPU on debian (my case) for
instance.
But as I said, I can not be full time on this so I have to do it
incrementally to prepare the big changes
I want to introduce. So better keeping using Tulip at the moment but
expect some cool things to happen in that repo along the next year ;-)
Cheers !
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2?email_source=notifications&email_token=AANMAFIRIJH3VL6KFYTHF43QY77F7A5CNFSM4JZOSOYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHALP3A#issuecomment-566278124>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANMAFNLWMPKXL3TFUP7C2DQY77F7ANCNFSM4JZOSOYA>
.
|
QOpenGL module is marked as deprecated since a while now so it is time to remove its use from the Talipot codebase and promote the use of QOpenGL* classes directly integrated in the QtGui module. The big difference between QOpenGL and QtOpenGL from Qt5 is that all rendering is performed in framebuffer objects, there is no more direct rendering in the underlying os windows with its own OpenGL context. Talipot OpenGL rendering also follows that idiom, all renderings are performed offscreen using a shared OpenGL context. This also means that there is no more QGLWidget as viewport for QGraphicsView. Talipot OpenGL scene are now converted to QImage in order to display them using the default Qt raster rendering engine. This should fixes the numerous rendering glitches observed on MacOS. First thing observed after the migration is a consequent performance boost in OpenGL rendering when using an Intel GPU on a Linux host machine (especially when selecting elements, it is now 10 times faster on debian stable).
I had an issue with some generated graphml that contained multiple entries with the same name (and sometimes different types), which crashed the import. The fix only prepends '_' to existing key names.