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

Bug: Many XML regions are not shown #127

Closed
rgleason opened this issue Jun 22, 2022 · 52 comments
Closed

Bug: Many XML regions are not shown #127

rgleason opened this issue Jun 22, 2022 · 52 comments

Comments

@rgleason
Copy link
Owner

rgleason commented Jun 22, 2022

@ozolli @bdbcat
This bug was noticed in 1.9.37 while testing the new "Retrieve > Update Data Sources" which works well.
This bug has been around for awhile, and started before version 1.9.31
I had not noticed it and have not traced it back further than that yet.

For some reason NOAA only shows "Boston" and none of the other regions, Pt. Reyes, New Orleans, Kodiak, Honolulu
NOAA-OPC similarly is not complete.
Every single Server is coming up short on Regions, missing many many weatherfaxes.

Why?
I am wondering if the "/" used in the Region Names is the problem? Was "/" that used before?

@rgleason
Copy link
Owner Author

Now I don't think the "/" is the problem, I went back to look at the xml in 1.9.30.

@ozolli
Copy link

ozolli commented Jun 22, 2022

Well, I cannot really help here as it works fine for me....

@rgleason
Copy link
Owner Author

rgleason commented Jun 22, 2022

You have MacOS? Did you change anything in the Region Titles from "-" to "/" if you recall??

@ozolli
Copy link

ozolli commented Jun 22, 2022

No, Linux Ubuntu or RPi4 or Windows 10.

@rgleason
Copy link
Owner Author

rgleason commented Jun 22, 2022

Are you sure it is working "fine". Does NOAA show all the regions?
Boston, Pt. Reyes, New Orleans, Kodiak, Honolulu

Mine shows only "Boston" region Many others are missing too.

@ozolli
Copy link

ozolli commented Jun 22, 2022

Yes, all the regions, and I had to remap all of them for the low res version.

@rgleason
Copy link
Owner Author

Which coorindateSets.xml are you using?

@ozolli
Copy link

ozolli commented Jun 22, 2022

Hi res

@ozolli
Copy link

ozolli commented Jun 22, 2022

Same with normal res.
You can't download the file or you can't display it ?

@rgleason
Copy link
Owner Author

I am using Weatherfax 1.9.37.0 as installed by PIM. I have checked both install dir and user dir and the coordinateSets.xml file are the same. I don't think that is the problem. Also Retrieve > Update Data Files works fine.

Not all Regions are displaying.. many of them. We are also missing India. The files exist. The xml files show these regions.
It is bizarre.

PWx-Euro-Atlantic
Aviation
PW-amer-atlantic
NOAA-OPC-00Z
NOAA-OPC-Atlantic-only
NOAA-Boston-only
Arome
Arpage
UKmet
GFS
ECMWF
GermanWeather

@rgleason
Copy link
Owner Author

This is frustrating. I thought we were done with this plugin.

@ozolli
Copy link

ozolli commented Jun 22, 2022

I see that you miss some regions in the Aviation Weather server.
WFIR_Aviation_Weather.xml and the respective lines in CoordinatesSets.xml haven't changed for at least 13 months.
If you revert to, say, v1.9.20.0, do you still have this bug ?

@ozolli
Copy link

ozolli commented Jun 22, 2022

I found a bug in 1.9.37 that is not present in 1.9.36 and that can be related to what you see:
In 1.9.36 when I click on a server which has only one region, i.e. French Polynesia or Chile, then when I click on the region I see all the charts.
In 1.9.37 I see nothing when clicking on the region.
However if I click the "All" button under the Regions list then when I browse to the region I see the charts.

Edit: This is not related to 1.9.37. This bug appears after having updated the data sources.

Edit2: In fact it is. I wiped anything and reinstalled WF 1.9.37: Bug is present.

@rgleason
Copy link
Owner Author

It appears to me that WIFR_Aviation_Weather.xml is the only one that really works with all five of the Regions showing!
Infrared, Infrared color, Infrared-NWS-color, Visible and Water-vapor.
Aviation

Just saw your post above.... Yes, now you've found the issue.

@ozolli
Copy link

ozolli commented Jun 22, 2022

Let me check some things and report again.

@rgleason
Copy link
Owner Author

rgleason commented Jun 22, 2022

Either the program is not reading the xml files properly, or the xml files have some very small issue. I think.
I don't know why I did not see this before, I was just going to check the local weather faxes and found I was missing things that should have been there.

Later: Thanks Ozolli for checking.

@ozolli
Copy link

ozolli commented Jun 22, 2022

Well, I'm not sure now.
I wiped all WF data, reinstalled 1.9.36, updated data sources and it works fine.
The bug is in 1.9.37 but I don't believe in the data directory.

@ozolli
Copy link

ozolli commented Jun 22, 2022

It appears to me that WIFR_Aviation_Weather.xml is the only one that really works with all five of the Regions showing!
Infrared, Infrared color, Infrared-NWS-color, Visible and Water-vapor.

Yes but in each region you miss several charts: Americas, Indian Ocean and Oceania, Pacific, North Pacific.

@rgleason
Copy link
Owner Author

rgleason commented Jun 22, 2022

I find that 1.9.36 has the same issues after removing
C:\ProgramData\opencpn\plugins\weatherfax
C:\ProgramData\opencpn\plugins\cache ( removing weatherfax metadata)
%localappdata%\opencpn\plugins\weatherfax_pi OR C:\Users\fcgle\AppData\Local\opencpn\plugins\weatherfax_pi
removing the weatherfax_pi.dll from C:\Users\fcgle\AppData\Local\opencpn\plugins
and installing v1.9.36

At least we agree there is an issue.

@rgleason
Copy link
Owner Author

In 1.9.36 Click NOAA, and Boston comes up in Regions (missing 5 other regions), then click "ALL"
Just Boston shows, plus a lot of other regions, but not Pt Reyes, New Orleans, Honolulu, or Kodiak Alaska!
Screenshot (20)

@rgleason
Copy link
Owner Author

If you use Retrieve > Update data files you will be updating to the current version 1.9.37 data.
We could go back to an earlier version and try to use the current data and see if it still has a problem.

@rgleason
Copy link
Owner Author

rgleason commented Jun 22, 2022

I removed the weatherfax directories, data and using Import tarball installed 1.9.32.5 and tried it. I has the same issues with a clean installation. Maybe you can try?
weatherfax_pi-1.9.32.5-msvc-x86_64-10.0.14393-MSVC.tar.gz

@rgleason
Copy link
Owner Author

Now I tried updating the Master Catalog and downloading and installing 1.9.37 and the enabling it.
Now it is working! Bizarro.

image

@rgleason
Copy link
Owner Author

I think I know what the issue is now. The NOAA servers are very slow today, they have one shut down.
What a waste of time. I am sorry.
Here are some other screenshots where the rest of the Regions appear, but very very slowly. You must be patient.
The regions don't seem to appear unless they actually work?
Screenshot (25)
Screenshot (26)
Screenshot (27)
Screenshot (28)

I think we may have identified the actual issue is NOAA servers being slow today.

@ozolli
Copy link

ozolli commented Jun 22, 2022

Tomorrow I will test again 1.9.36 vs 1.9.37 because I am not so sure the NOAA slow servers are the only issues here.

Edit: Hope i'm wrong ;-)

@rgleason
Copy link
Owner Author

I will leave this open

@rgleason rgleason reopened this Jun 22, 2022
@ozolli
Copy link

ozolli commented Jun 23, 2022

@rgleason @bdbcat

I tested again 1.9.36 vs 1.9.37.
I made the screenshots from Windows because the native linux version of OpenCPN has the invisible selection bug (#107).

v1.9.37. When opening the intenet download window we get this:
1

All the servers and all the regions are already selected as if we had clicked the "All" servers and "All" regions buttons.

Then I clicked on "French Polynesia" server:
2

The "South Pacific" region is selected and we can see all the available charts.

Now click on "Chile" server:
3

The Chile region is selected but we cannot see the available charts.

Finally click on "French Polynesia" server again:
4

The "South Pacific" region is selected but this time we cannot see all the available charts.

This behaviour does not exist on v1.9.36. When opening the internet download window ony one server and one region is selected and the available charts are seen:
36

@rgleason
Copy link
Owner Author

rgleason commented Jun 23, 2022

Do you have any thoughts on why this is happening? I have looked through the code, and don't see that any of the recent would cause this. The only thing I can surmise is that we have multiple WFIR_ files that are read and displayed as if they are one XML file and there is some glitch in the coding for this which accounts for the failure of the xml to display. Perhaps it is a missing return character or space when the mulitple xml's are collected together to be displayed, or something like that. Is that plausible?

@ozolli
Copy link

ozolli commented Jun 23, 2022

I have not looked into the code yet and don't know if I'm qualified to understand it sufficiently.
TBH I tagged @bdbcat as he published the PR that could be the culprit...

@rgleason
Copy link
Owner Author

rgleason commented Jun 23, 2022

Ozolli, actually this is the commit list
https://github.com/rgleason/weatherfax_pi/commits/master

  1. 1.9.36.0 update coordinateSets normal or high
  2. Then Dave's improvements Correct XML data update file destinations
  3. Merge of those improvements Merge branch 'rgleason:master' into master
  4. My increment of the version so that we can get the files into cloudsmith "prod" repository 1.9.37.0 bdbcat update data fix

You can drill down and see the actual changes, and I am having a difficult time figuring out what change is causing this problem. You've been a great help quantifying the issue. We should be able to identify the area in the code which is causing this.

@ozolli
Copy link

ozolli commented Jun 23, 2022

That's just an intuition, maybe this is here in src/InternetRetrievalDialog.cpp:

835 - m_lServers->SetSelection(m_lServers->Append(it->Name), it->Selected);
850 + m_lServers->SetSelection(m_lServers->Append(it->Name));

(same for 858/873)

@rgleason
Copy link
Owner Author

Thanks. Perhaps Dave will have a chance to look at this.

@ozolli
Copy link

ozolli commented Jun 24, 2022

I compiled the plugin with
m_lServers->SetSelection(m_lServers->Append(it->Name), it->Selected);
and it works fine now.

@rgleason
Copy link
Owner Author

Thats great. You solved it. I will make that change

@bdbcat
Copy link
Collaborator

bdbcat commented Jun 24, 2022

ozolli...
Good catch. Well done.
Dave

@rgleason
Copy link
Owner Author

@ozolli

I compiled the plugin with
m_lServers->SetSelection(m_lServers->Append(it->Name), it->Selected);
and it works fine now.

Is this change made on LINE 850? In lieu of

850 + m_lServers->SetSelection(m_lServers->Append(it->Name));

BTW I do not find this at line 835 or anywhere near line 835
835 - m_lServers->SetSelection(m_lServers->Append(it->Name), it->Selected);

I find with a search "m_lServers->SetSelection" that line 187, 515 and 850 have that search, but only 850 has the full line

Can you explain further please? Thanks.

@ozolli
Copy link

ozolli commented Jun 25, 2022

Rick,
The other one is on line 873.

@rgleason
Copy link
Owner Author

yes
873 m_lRegions->SetSelection(m_lRegions->Append(it->Name));

is this line supposed to be

873 m_lServers->SetSelection(m_lServers->Append(it->Name), it->Selected);

???

@ozolli
Copy link

ozolli commented Jun 25, 2022

No you keep m-lRegions. Just add: , it->Selected

m_lRegions->SetSelection(m_lRegions->Append(it->Name), it->Selected);

@rgleason
Copy link
Owner Author

OK ozolli, some confusion on my part.
I have made both Line 873 and 850 the same

m_lServers->SetSelection(m_lServers->Append(it->Name), it->Selected);

and built and push it up. Please be explicit so this can be done efficiently. I don't like having a bunch of garbage in the commit history. For example, like this??? Many variations

Line 850     m_lServers->SetSelection(m_lServers->Append(it->Name), it->Selected);
Line 873     m_lRegions->SetSelection(m_lRegions->Append(it->Name, it->Selected); 

@rgleason
Copy link
Owner Author

Sorry I am leaving to go sail. Will fix this in a couple of days.

@ozolli
Copy link

ozolli commented Jun 25, 2022

Sorry Rick, I should have been more explicit.
This is correct:

Line 850 m_lServers->SetSelection(m_lServers->Append(it->Name), it->Selected);
Line 873 m_lRegions->SetSelection(m_lRegions->Append(it->Name, it->Selected);

@rgleason
Copy link
Owner Author

This seems to work, sort of, changing just the first line 850, but "All" "None" commands don't work right and single regions do not display.
Screenshot (37)

But when I add line 873 it does not build locally and I get errors.

Screenshot (39)

 aes.cpp
C:\Users\fcgle\source\weatherfax_pi\src\InternetRetrievalDialog.cpp(873): error C2664: 'int wxItemContainer::Append(const std::vector<wxString,std::allocator<_Ty>> &)': cannot convert argument 2 from 'bool' to 'void *' [C:\Users\fcgle\source\weatherfax_pi\build\weatherfax_pi.vcxproj]
          with
          [
              _Ty=wxString
          ]
  C:\Users\fcgle\source\weatherfax_pi\src\InternetRetrievalDialog.cpp(873): note: Conversion from integral type to pointer type requires reinterpret_cast, C-style cast or function-style cast
C:\Users\fcgle\source\weatherfax_pi\src\InternetRetrievalDialog.cpp(873): error C2143: syntax error: missing ')' before ';' [C:\Users\fcgle\source\weatherfax_pi\build\weatherfax_pi.vcxproj]
  af_vfs.cpp
  compression.cpp

@rgleason
Copy link
Owner Author

rgleason commented Jun 26, 2022

@ozolli @bdbcat So what are the lines used in your working version please?

@rgleason
Copy link
Owner Author

I added the necessary ")" at the end before the ";" and I still get this error.

 Setup.cpp
  Track.cpp
C:\Users\fcgle\source\weatherfax_pi\src\InternetRetrievalDialog.cpp(873): error C2664: 'int wxItemContainer::Append(const std::vector<wxString,std::allocator<_Ty>> &)': cannot convert argument 2 from 'bool' to 'void *' [C:\Users\fcgle\source\weatherfax_pi\build\weatherfax_pi.vcxproj]
          with
          [
              _Ty=wxString
          ]
  C:\Users\fcgle\source\weatherfax_pi\src\InternetRetrievalDialog.cpp(873): note: Conversion from integral type to pointer type requires reinterpret_cast, C-style cast or function-style cast
  UUID.cpp
  WAVE.cpp
  aes.cpp
  af_vfs.cpp
  compression.cpp
  data.cpp
  debug.cpp
  format.cpp
  openclose.cpp
C:\Users\fcgle\source\weatherfax_pi\src\libaudiofile\WAVE.cpp(1135): warning C4267: '=': conversion from 'size_t' to 'uint16_t', possible loss of data [C:\Users\fcgle\source\weatherfax_pi\build\weatherfax_pi.vcxproj]
C:\Users\fcgle\source\weatherfax_pi\src\libaudiofile\WAVE.cpp(1142): warning C4267: '=': conversion from 'size_t' to 'uint16_t', possible loss of data [C:\Users\fcgle\source\weatherfax_pi\build\weatherfax_pi.vcxproj]
C:\Users\fcgle\source\weatherfax_pi\src\libaudiofile\WAVE.cpp(1182): warning C4267: 'initializing': conversion from 'size_t' to 'uint16_t', possible loss of data [C:\Users\fcgle\source\weatherfax_pi\build\weatherfax_pi.vcxproj]
C:\Users\fcgle\source\weatherfax_pi\src\libaudiofile\WAVE.cpp(1189): warning C4267: 'initializing': conversion from 'size_t' to 'uint16_t', possible loss of data [C:\Users\fcgle\source\weatherfax_pi\build\weatherfax_pi.vcxproj]
  PacketTable.cpp
  query.cpp

@rgleason
Copy link
Owner Author

I am going to remove weatherfax from the catalogs.

@rgleason
Copy link
Owner Author

Also I notice there is a loose "} in this procedure and the { and } aren't paired properly.

@rgleason
Copy link
Owner Author

I was wrong, the { and } do match up. It it the conversion from integral to pointer type...

@ozolli
Copy link

ozolli commented Jun 27, 2022

Here is the file I modified :
InternetRetrievalDialog.zip

Yes there was a missing ) on line 873 in the above example. It should be :
Line 873 m_lRegions->SetSelection(m_lRegions->Append(it->Name), it->Selected);

On linux native ubuntu-x86_64-22.04 everything works well : Single regions, all, none.

@rgleason
Copy link
Owner Author

Thanks, that fixed it. I had located the ) in the wrong place.
3073ad1

@ozolli
Copy link

ozolli commented Jun 27, 2022

1.9.40 is perfect here !

@rgleason
Copy link
Owner Author

Good! will push to opencp/plugins now

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

No branches or pull requests

3 participants