FLO-2DSoftware / qgis-flo-2d-plugin

A plugin for pre-processing/post-processing FLO-2D models
5 stars 7 forks source link

Save Project Issue #1342

Closed FLO-2DKaren closed 1 month ago

FLO-2DKaren commented 1 month ago

Hi Robson,

When a new table is added to the gpkg, the plugin automatically runs the Porting code. This is a problem when there external layers in the old gpkg.

For example, storm drain layers committed to the gpkg are editable and may not match original shapefiles so users risk losing work because we are not preserving those layers.

But I don't think this is a simple fix. It's going to take some thinking because I move my layer out of External Layers.

rpachaly commented 1 month ago

A potential solution is to create a table on the geopackage that stores the names of the layers that were externally added and then account for them on the porting code.

Let me finish the https://github.com/FLO-2DSoftware/qgis-flo-2d-plugin/issues/1141 and then I'll do this one.

This is important.

FLO-2DKaren commented 1 month ago

Yes it's important and I'm sure you have some ideas brewing. I do too. Let's do a meeting and some videos on this one.
I think we'll need to document this better in the User Instructions. It's great work but it will be confusing to most people until they get used to it. Especially since the old data is still in the shapefile and raster form.

rpachaly commented 1 month ago

Ok! I'll let you know when the swmm control issue is done and then we have the meeting to discuss this.

FLO-2DKaren commented 1 month ago

Hi Robson, I found some things for our save issues testing.

Robson is going to try an if statement to revise how the mapcrafter directory is searched.

https://github.com/FLO-2DSoftware/qgis-flo-2d-plugin/assets/20424460/ae8fec18-8c25-4a19-943c-120a6c637ded

FLO-2DKaren commented 1 month ago

I'm trying to build a project for you and I switched to my laptop and found another error.

https://github.com/FLO-2DSoftware/qgis-flo-2d-plugin/assets/20424460/ed0c1d5b-2a9a-4a1a-8378-ddf312ddc18f

rpachaly commented 1 month ago

This seems to be related to the settings. I'll try to have it fixed asap.

FLO-2DKaren commented 1 month ago

I'm really glad I did this test on my laptop because it means I have to do things that aren't normal.

Steps to cause error:

  1. Load project
  2. Load storm drain shapefiles.
  3. Close project - Like when you close a project because you want to reload the plugin.
  4. When asked click save

Error occurs I think because we don't finish the secondary save.

An error has occurred while executing Python code:

AttributeError: 'NoneType' object has no attribute 'dataProvider' Traceback (most recent call last): File "C:\Users/karen/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\flo2d\flo2d.py", line 1568, in load_gpkg_from_proj if not dlg_settings.connect(old_gpkg): ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "C:\Users/karen/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\flo2d\gui\dlg_settings.py", line 431, in connect self.lyrs.load_all_layers(self.gutils) File "C:\Users/karen/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\flo2d\layers.py", line 2292, in load_all_layers lyr_id = self.load_layer( ^^^^^^^^^^^^^^^^ File "C:\Users/karen/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\flo2d\layers.py", line 1843, in load_layer lyr_exists = self.layer_exists_in_group(uri, group) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "C:\Users/karen/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\flo2d\layers.py", line 2209, in layer_exists_in_group if normpath(lyr.layer().dataProvider().dataSourceUri()) == normpath(uri): ^^^^^^^^^^^^^^^^^^^^^^^^ AttributeError: 'NoneType' object has no attribute 'dataProvider'

FLO-2DKaren commented 1 month ago

@rpachaly I'm trying things that I don't normally try because I'm on my laptop. All good testing. Don't be frustrated by this issue. We know how important this testing is.

rpachaly commented 1 month ago

I'm not frustrated! I'm happy that we found this before the release!

FLO-2DKaren commented 1 month ago

Testing Project is here: Self Help Kit_v1.0.0.gpkg.zip

rpachaly commented 1 month ago

When the MapCrafter has an ID, it's being added to the geopackage.

I could not reproduce this one. I tested in many ways and it is not being saved on the geopackage.

KJ response "I'll try to reproduce this one. I know what causes it. It's when you try to load a project that already has a mapcrafter folder. Maybe I'm overthinking how I do that."

rpachaly commented 1 month ago

It took me the whole day to figure out how to do this, but now external layers (vectors/rasters) are being ported with the geopackage and added back to the map. Still need to do some tests tho, but it seems to be working good.

Only drawback is that it will be really complicated to keep the same symbology and add them in the same groups. Currently, they are added to the External Group. It can be done, but I'll spend some days on it. I don't think it worth the effort now.

I'll fix the other bugs reported in this issue before pushing the branch.

rpachaly commented 1 month ago

I could not reproduce this one

Close project and Save causing an issue.

rpachaly commented 1 month ago

I'm pushing the branch. Let's have a meeting this week to discuss the close issue.

https://github.com/FLO-2DSoftware/qgis-flo-2d-plugin/assets/39889306/e40ccdc3-277c-4586-9d18-58371768c033

https://github.com/FLO-2DSoftware/qgis-flo-2d-plugin/pull/1354

FLO-2DKaren commented 1 month ago

Hi Robson, I watched the video carefully and I think this is appropriate. I don't think others will have as many issues with this that we do since they will get a more complete version.

FLO-2DKaren commented 1 month ago

I found one more issue related to this. When you add a WCS server connected layer, it is not an x y z. We need to add that to the check table. I just noticed this when I connected to the 3dep server.

rpachaly commented 1 month ago

I check based on the layer provider now. See check 2

        # Check 1: Path cannot be equal to gpkg_path
        if gpkg_path_adj in layer_source_adj:
            return False

        # Check 2: Check based on the provider if the layer is raster or vector
        providers = ['ogr', 'gpkg', 'spatialite', 'memory', 'delimitedtext', 'gdal']
        if layer.dataProvider().name() not in providers:
            return False

        # Check 3: Check if it is an online raster or located in a MapCrafter folder
        if "MapCrafter" in layer.source():
            return False

        # Check 4: If the file is a raster
        if isinstance(layer, QgsVectorLayer):
            return True

        # Check 5: If the file is a vector
        if isinstance(layer, QgsRasterLayer):
            return True

Now all layers obtained from servers are not going to be saved in the gpkg.

I added a commit to the pull request.

FLO-2DKaren commented 1 month ago

Haha, I'm such an idiot. I've was watching your video of QGIS loading but it was paused. I was like why is this taking so long!!!

FLO-2DKaren commented 1 month ago

I tested this with many different file types. One weird thing I saw is that when I remove and elevation raster, it isn't removing the data. It's still the same size. Maybe it needs to be refreshed?

rpachaly commented 1 month ago

Is the file still on the geopackage?

FLO-2DKaren commented 1 month ago

I can't see it on the geopackage using db browser or Qgis Browser.

rpachaly commented 1 month ago

Weird. Let me try it here.

FLO-2DKaren commented 1 month ago

I removed it using the FLO-2D GeoPackage manager

Did you add a trigger so that when I use Remove layer that it triggers the Drop Table too? If so, that's cool.

rpachaly commented 1 month ago

Yes, it was keeping the disk allocated because this is how sql databases work.

'When you delete data from the database, the space used by that data is flagged as "free" inside the database file and is available for use by new data rows. The physical file never gets reduced unless you manually shrink the file/database.'

SQLite databases can use the 'VACUUM' command.

The VACUUM command rebuilds the database file, repacking it into a minimal amount of disk space.

I added this command and it looks like it is working fine when a table is dropped by the FLO-2D GeoPackage manager

I'm adding this commit so you can test it.

FLO-2DKaren commented 1 month ago

I didn't find any other major issues with my testing. Is there any reason to continue testing? If not, I'll go back to the SD Controls so I can update some images.

rpachaly commented 1 month ago

No, I think it is ok for merging.

FLO-2DKaren commented 1 month ago

OK I'll merge it. Thanks Robson.