simulton / QSchematic

A library that allows creating diagrams such as flowcharts or even proper engineering schematics within a Qt application.
https://simulton.com
MIT License
234 stars 60 forks source link

v2: grid size not expanding like expected? rewrite grid as bottom layer. #59

Closed bjornstromberg closed 5 months ago

bjornstromberg commented 8 months ago

not sure if this is a 'real' or 'cosmetic' bug since i have no wires yet, but the grid is what the wires attach to..

background grid not complete

i've made a minimal test with a rect item just to be able to move it around, and see if the grid updates correctly.

unfortunally its kind of random when it updates but it never fills the full sheet.

so is this a big problem? i think its just a cosmetic one, make the page larger and zoom so the grid fills the usable area is a working workaround.

however it should be adressed when time presents itself for it, due to the fact it does not look to nice.

i cant remeber if it looked the same on ubuntu-20.04 with Qt5.12 and QSchematic <1.5 since that was quite a while back.

hopefully @Tectu can fill in if this is anything to worry about in the short term.

minimal test:

CMakeList.txt

project(qschematic-issue-59 VERSION 0.1)

set(CMAKE_CXX_STANDARD 20)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

# make sure newest possible Qt6 is used, but disregard Qt7 when that day eventually comes!
set(CMAKE_FIND_PACKAGE_SORT_ORDER NATURAL)
set(CMAKE_FIND_PACKAGE_SORT_DIRECTION DEC)
find_package(Qt6 6.5...<7 REQUIRED Widgets)

find_package(QSchematic 1.5 REQUIRED)

add_executable(${PROJECT_NAME} main.cxx)

target_link_libraries(${PROJECT_NAME} PRIVATE Qt6::Widgets qschematic::qschematic-static)

include(GNUInstallDirs)
install(TARGETS ${PROJECT_NAME})

main.cxx

#include <QtCore/QPointer>

#include <QtWidgets/QApplication>
#include <QtWidgets/QMainWindow>

#include "qschematic/items/rectitem.hpp"
#include "qschematic/scene.hpp"
#include "qschematic/view.hpp"

int
main(int argc, char **argv)
{
  QApplication app {argc, argv};

  QPointer<QSchematic::View> p_schematic_view {new QSchematic::View()};
  QPointer<QSchematic::Scene> p_schematic_scene {new QSchematic::Scene()};
  p_schematic_scene->addItem(std::make_shared<QSchematic::Items::RectItem>(QSchematic::Items::Item::ItemType::NodeType));
  p_schematic_view->setScene(p_schematic_scene);

  QMainWindow win;
  win.setCentralWidget(p_schematic_view);
  win.setMinimumSize(QSize(800, 600));
  win.show();

  return app.exec();
}

Tectu commented 8 months ago

I think that happens because you did not set the initial scene size and the underlying QGraphicsScene is re-sizing after you added the item. See QGraphicsScene::setSceneRect(). For example:

p_schematic_scene->setSceneRect(0, 0, 1000, 1000);

The QSchematic::Scene certainly connects to the QGraphicsScene::sceneRectChanged signal to re-render the background when necessary.

On a side note: I'm not too happy about the way the grid is being rendered by QSchematics. As of today, we are rendering a QPixmap spanning the entire scene background. That is horribly inefficient, leads to an exploding QPixmap size and also means that zooming in provides you with a crunchy rastered set of points. I'm not sure what the best approach for this is but I do feel that the grid points should probably just be QGraphicsItems that we handle in a separate "layer". The underlying Qt graphics framework would then make sure that we only render when necessary rather than pre-rendering every grid point and using it as a background image.

bjornstromberg commented 8 months ago

I think that happens because you did not set the initial scene size and the underlying QGraphicsScene is re-sizing after you added the item. See QGraphicsScene::setSceneRect(). For example:

p_schematic_scene->setSceneRect(0, 0, 1000, 1000);

The QSchematic::Scene certainly connects to the QGraphicsScene::sceneRectChanged signal to re-render the background when necessary.

yea i wanted to avoid that since the size of the diagrams can vary in exteme ways, think A5 -> A1 -> 2 or 4A0 blueprints

one solution would be to start with A5 and when its to small size up one size and go to A4 [...] it would be a simple solution. but the pixmap you wrote about will likely be substancial when it gets large.

On a side note: I'm not too happy about the way the grid is being rendered by QSchematics. As of today, we are rendering a QPixmap spanning the entire scene background. That is horribly inefficient, leads to an exploding QPixmap size and also means that zooming in provides you with a crunchy rastered set of points. I'm not sure what the best approach for this is but I do feel that the grid points should probably just be QGraphicsItems that we handle in a separate "layer". The underlying Qt graphics framework would then make sure that we only render when necessary rather than pre-rendering every grid point and using it as a background image.

oh pixmap?! that explains why the rectangular updates of dots goes bananas when it gets bigger.

my assumption was that it was a single untouchable qgraphics item that just painted the dots and kept hidden from touching it. kind of what you describe, that was how i though it was implemented (without actually digging though the code for the answer) since qpainter support painting dots it was just my assumption.

but that code change can hardly be a breaking abi one to create a dot sheet as bottom layer in the QGraphicsScene and force it to update on every scene-resize. to QSchematic Scene everything is still null parent if the bottom layer, then make sure the z axis is higher then the dot sheet.

wont promise anything but i might look into this if this issue irritates me enough.

Tectu commented 8 months ago

Contributions are certainly welcomed :)

bjornstromberg commented 8 months ago

Contributions are certainly welcomed :)

where in the codebase is the gridsize defined? since we got snap to grid, the grid size has to be defined somewhere.. :roll_eyes:

eg. if its currently 50 x 50 and it has 2.5px between each dot, and how large is the dots.. when this is extended to 500 x 500 and still is 2.5px or is it 10px by now?

i think the grid should auto resize, based on zoom, cause eventually it becomes to small to see, or to big jumps, but this will likely mess with snap to grid..

Tectu commented 8 months ago

where in the codebase is the gridsize defined? since we got snap to grid, the grid size has to be defined somewhere.. 🙄

QSchematic::Settings::gridSize

i think the grid should auto resize, based on zoom, cause eventually it becomes to small to see, or to big jumps, but this will likely mess with snap to grid..

Definitely, yes. There are plenty of features that would be nice to have. Any contribution is very welcomed :)

The whole QSchematic::Settings thing is a mood point anyway. We'd like to have much better control than what is currently there. Ideally there would be some support for "stylesheets"/"themes" and similar. Right now Settings is a very monolithic piece of bad design.

bjornstromberg commented 8 months ago

assign this on me and put it on the v2 milestone then, i guess i'll have to break abi on all sorts of stuff, the settings is one of those.

timeframe? well depends on how much of a rewrite it will become..

i guess you have alot of ideas and improvements that we need to get down in a todo list

i have earlier today setup a branch for v2 dev i think we should do the same here, so we got a dev-2 or master-2 or something so that branch lives its own life until its ready.

bjornstromberg commented 8 months ago

i have earlier today setup a branch for v2 dev i think we should do the same here, so we got a dev-2 or master-2 or something so that branch lives its own life until its ready.

another solution could be that we make a 'release-1' branch here that tracks the old version and its maintenence, and the path towards v2 with breaking changes goes into master.

@Tectu your the release engineer on this project so your the one that sets the path of how you want it, you maybe want the feature releases of v1.x to incorporate the stuff that goes into final v2 directly since everyone using this library as a static library anyway?

Tectu commented 8 months ago

I'm pretty much okay with everything. This is a small, non-mission/non-safety critical library that most consumers decide to link to statically or consuming it via CMake's FetchContent_Declare(). Consumers of the shared library can always restrict the version range as needed.

As such, my strategy so far was to keep patch versions compatible and to keep changes between minor versions "manageable". There is no hard ruling on what "manageable" means that but if you have a look at the change(s) of 1.5.0 you'll notice that it's pretty much just a search & replace exercise for any consumer from an API/source-code point of view.

I have no problem continuing like that. Creating a release/1.5.1 branch and keeping the master branch for v2 efforts is fine too.

bjornstromberg commented 8 months ago

As such, my strategy so far was to keep patch versions compatible and to keep changes between minor versions "manageable". There is no hard ruling on what "manageable" means that but if you have a look at the change(s) of 1.5.0 you'll notice that it's pretty much just a search & replace exercise for any consumer from an API/source-code point of view.

so from your perspective a 1.6 release could contain #57 & #61 without issues? as i see it getting those two in are the breakers on pretty much everything for getting forward to v2, the rest of the changes should pretty much be just be added functions or is hidden behind the 'firewalls' unless directly used..

it sounds like a sound strategy to minimize the maintenence burden.

I have no problem continuing like that.

if you have no requirements on actually not breaking abi on 1.5 series i would say 1.6 is just a milestone towards 2.0.

Creating a release/1.5.1 branch and keeping the master branch for v2 efforts is fine too.

minimal maintenece burden is best path forward.

reason for getting the path forward clear now is that i want to automate the upstreaming process since there is likely going to take a while to get everything done, that way it will eventually actually get done..

if everything goes into master then that will be alot easier task than refactoring stuff that goes into both rel/1.5 and master (v2)

worst case scenario is that the work does never actually get upstreamed, cause it gets stuck as pull requests getting out of date, back and forth, and we're back to someone else reinventing the wheel, I'm sure there are better things for people to do then keep reinventing the wheel..

it certainly is for me, thats why i started from your library.

Tectu commented 8 months ago

I think a reasonable way would be to implement #44, #57 and #61 and call that the 2.0.0 release. From there, we can continue with the previous "manageable breakage between minor versions" approach.

bjornstromberg commented 8 months ago

I think a reasonable way would be to implement #44, #57 and #61 and call that the 2.0.0 release. From there, we can continue with the previous "manageable breakage between minor versions" approach.

works for me :+1: