Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

little improvements in composer module #33

Closed
wants to merge 3 commits into from

Conversation

rokemoon
Copy link

removed duplicate code, openGL for drawing (if support of course), removed the unnecessary creation and deletion of QGraphicsRectItem.

@rokemoon rokemoon closed this Aug 1, 2011
@rokemoon rokemoon reopened this Aug 1, 2011
@rokemoon
Copy link
Author

rokemoon commented Aug 1, 2011

Maybe someone have any question about changes?

{
mComposition->addItemToZList( this );
}
init(manageZValue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to extract all the stuff from the constructor out into a init method?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, because we use init in 2 constructor, thereby remove duplication.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see now, makes sense.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glad to hear it

@wonder-sk
Copy link
Member

I am quite curious about the optimization you have done in composer / map canvas. Can you comment somehow the performance with/without using OpenGL widget, e.g. measure the time required for rendering?

Because using OpenGL does not automatically mean that something will be faster - only certain usage patterns result in faster and therefore I would like to see a proof that these optimizations make sense. Also not everyone has the hardware acceleration working, so it might be interesting also to see the performance with software emulation.

@rokemoon
Copy link
Author

rokemoon commented Aug 1, 2011

I apologize for the rant. I mean optimization in QgsComposerItem where i delete unnecessary deletion and creation of QGraphicsItem. About OpenGL, right now I work on project where we have a lot of items over the mapCanvas, they all can move and resize (changes occur immediately <<- in this moment we lose in productivity), so when we have count of items about 200 we observed no long delays with software emulation. Then I use OpenGl and count of items when we have dalys is about 4 000, all is fine moving them or resizing are very fast. Therefore I thought it would be nice to see such results in Composer Module of QGis, because in this module user can change items and can have many items.
"Also not everyone has the hardware acceleration working" for that case I set ifdef, oh but I forgot about CmakeList. About performance maybe for QGis it unnecessary (no sarcasm), because items have idly changing (pending changes). Most likely this is useful for our case, well it can be undone.

@rokemoon rokemoon closed this Aug 2, 2011
@mhugent
Copy link
Contributor

mhugent commented Aug 2, 2011

Hi Rokemoon

Just back from the weekend. I'm going to review the changes during the next days.

Regards,
Marco

@rokemoon rokemoon reopened this Aug 2, 2011
@rokemoon
Copy link
Author

rokemoon commented Aug 2, 2011

Hello mhugent.
Ok, I welcome your comments.

@mhugent
Copy link
Contributor

mhugent commented Aug 3, 2011

Hi Rokemoon

I also think we should stay with the default paint engine. It is a very rare case to have several hundreds of items in a composer layout. With a new paint engine, there is the possibility to have new problems (e.g. the fonts in the composer scalebar and in the labels don't appear nicely with the open gl engine on my computer).

Moving the duplicated content of the constructors to an init() method and replacement of it/else with switch is fine, so I'm going to commit that part of the pull request.

Thanks for your contribution,
Marco

@mhugent mhugent closed this Aug 3, 2011
@rokemoon
Copy link
Author

rokemoon commented Aug 3, 2011

Sorry, but what about unnecassery deletion and creation of QGraphicsRectItem in mouseEvent?

@mhugent
Copy link
Contributor

mhugent commented Aug 3, 2011

Creation of QGraphicsRectItem is very fast, so I don't see a reason to store an item on demand for every existing composer item.

@qgib qgib mentioned this pull request May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants