cds-astro / aladin-lite

An astronomical HiPS visualizer in the browser
https://aladin.cds.unistra.fr/AladinLite/
GNU General Public License v3.0
100 stars 41 forks source link

A.catalogFromURL - Error during parsing VO Table #137

Closed szpetny closed 7 months ago

szpetny commented 7 months ago

Hi Matthieu,

We have a problem with A.catalogFromURL, it seems to collapse on parsing VO table.

Here is an example attached:

https://jsfiddle.net/3Lok0g2u/12/

Here is a newer example with 2 urls

https://jsfiddle.net/wg3uc62s/1/

I added there few aladin versions which apparently have this bug, and a working one at the end in HTML part.

szpetny commented 7 months ago

I compared the new vo table with the old working one and the new contains this field <FIELD datatype="long" name="sourceId" ucd="meta.id;meta.main">

I would even check it but currently have the same problem as ghansham when try to compile the project.

Anyways I am attaching the files with VO tables not-working.txt working.txt

bmatthieu3 commented 7 months ago

Hi @szpetny, @ghansham

Did it occur recently on the beta or it always failed ? I ask that because I recently worked on aladin lite versioning (see https://aladin.cds.unistra.fr/AladinLite/doc/release/ for all the versions)

Anyway, I wanted to send you a mail about the new UI work I did on aladin lite for you to test. It also seems that the coming version fixes that VOTable bug too: here is the link to the bundle for you to test the fix and the UI. https://aladin.cds.unistra.fr/AladinLite/alpha/aladin.js I will soon publish a draft API documentation for these new features and expect to update the latest with that new version in the days to come! If you discover new bugs you can always revert your aladin lite thanks to the release page and of course, open an issue for me to fix the new version: https://aladin.cds.unistra.fr/AladinLite/doc/release/

szpetny commented 7 months ago

Hi @bmatthieu3

Thanks for quick reply and the new version fixes the vo table bug indeed. And the new GUI looks very nice :)


For me I had some problems with building Aladin since version 3, because I don't know rust. But I am getting the very same errors as @ghansham reported on https://github.com/cds-astro/aladin-lite/issues/129 I tried to build with stable and nightly modes of rust. I'm using the latest version of rust and develop branch. From the logs it looks like source of the problem is color.rs


Also I'm getting weird characters instead of icons, I think must be some encoding problem. It was also like that in the previous version. The problem does not occur when I build the app myself. This is what I see: image image image

Not sure if it is only me?

bmatthieu3 commented 7 months ago

What is your command for compiling the rust ? Is the error the same when you do ?

cd src/core
cargo check --release --features=webgl2

It seems the character '▶' has some trouble running on your browser. We should maybe put an svg icon instead here.

szpetny commented 7 months ago

After executing these commands I've got kind of the same :( It looks like this:

image image


I think svg icon would do great :)

szpetny commented 7 months ago

I executed again because was not sure if the nightly rust mode was set previously. But with building using stable I'm getting this:

image

bmatthieu3 commented 7 months ago

This is strange, as you seem to use windows maybe it is related to an incorrect path ? In color.rs there is a link to the Color.js file written as:

#[wasm_bindgen(raw_module = "../../js/Color.js")]

Could it be possible that this path convention only work on unix system and not windows ? I am not sure.

szpetny commented 7 months ago

I mean in angular projects these kind of paths work. But you are right, perhaps it is windows thing, I will try later to build using wsl and let you know. Cheers

szpetny commented 7 months ago

@bmatthieu3 not sure, but perhaps the zoom controls (buttons) have a wrong class name (in the attached new Aladin version)?

ghansham commented 7 months ago

Hey guys, I struggled with this. Could not resolve it. If you can fix it, that would be a great help.

Regards Ghansham

On Mon, 4 Mar, 2024, 6:54 pm szpetny, @.***> wrote:

@bmatthieu3 https://github.com/bmatthieu3 not sure, but perhaps the zoom controls (buttons) have a wrong class name (in the attached new Aladin version)?

— Reply to this email directly, view it on GitHub https://github.com/cds-astro/aladin-lite/issues/137#issuecomment-1976574926, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYXFJHRFROTXGKHSYBWN53YWRYYNAVCNFSM6AAAAABEBY6KB2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZWGU3TIOJSGY . You are receiving this because you were mentioned.Message ID: @.***>

bmatthieu3 commented 7 months ago

@szpetny - I did put an svg icon for your menu encoding problem. It should fix that problem I hope. Tell me if it works for the path, I hope it is the problem. Also for the zoom buttons I realized they were always enabled even if showZoomControl: false when initializing the Aladin object. I fixed it. Is it what you thought ?

szpetny commented 7 months ago

@bmatthieu3 Matthieu, but is there a place I could get the new version of Aladin compiled? or is it the same link as previously (in that case the problem with the icon still exists)?

Unfortunately with the project building I went nowhere, I even couldn't compile it in wsl because I have a problem to install some dependencies for the rust. I'm still getting the same error.

About zoom controls - I thought the css class was different, but this is not relevant anymore, at least you fixed a bug :)

bmatthieu3 commented 7 months ago

@szpetny - You are right, I just pushed a new version on the same link (the alpha one).

For the building can you try maybe changing

#[wasm_bindgen(raw_module = "../../js/Color.js")]

into

#[wasm_bindgen(raw_module = ".\..\..\js\Color.js")]

The rust part has to locate the Color.js file someway

szpetny commented 7 months ago

@bmatthieu3 I've tested a whole bunch of possible paths and no success :sadpanda:

ManonMarchand commented 7 months ago

It compiles on my machine with the develop branch on 8330e916cb6add179a2da111f916198d35743e9e and the nightly rust toolchain. I had to

cargo install wasm-pack@0.12.1
git clean -fd

To get rid of some wasm-bingen errors though.

bmatthieu3 commented 7 months ago

@szpetny - Maybe you are also in a 'bad' compiling state. Maybe remove the src/core/Cargo.lock file and the src/core/target directory before trying to recompile with: cargo check --release --features=webgl2 from the src/core directory.

Also I still can modify the color.rs to not refer to that Color.js file, maybe that will help debug things.

szpetny commented 7 months ago

Hi @bmatthieu3 , thanks for help. I used the hints from you and @ManonMarchand (thanks :)) and yes that helped. I was able to compile rust core with some warnings.

I assumed when the core is built it is enough to call vite build from the root catalog.

So Aladin built but then when I try to test it (aladin.js) in my project I'm getting errors again like those below

image

bmatthieu3 commented 7 months ago

it seems maybe the bundling step is not done properly. Did you just try running from npm ?

# first install the dev depedencies for npm
npm install
# second run a build, this will first build the rust's core, call wasm-pack and then vite build. 
npm run build

What I have after that is a terminal capture like that:

image

Then you have 2 files generated in a dist directory at the root of the repo: aladin.js and aladin.umd.cjs. Also keep in mind that if you include aladin with a script html element then you need to use the aladin.umd.cjs and not the aladin.js. Aladin.js is for using Aladin in a npm package.

Example of a script located at the root of the repo looking for the dist/aladin.umd.cjs:

<!doctype html>
<html>
<head>
    <meta name="viewport" content="width=device-width, height=device-height, maximum-scale=1.0, initial-scale=1.0, user-scalable=no">
</head>
<body>
<div id="aladin-lite-div" style="width: 500px; height: 500px"></div>

<script type="text/javascript" src="./dist/aladin.umd.cjs" charset="utf-8"></script>
<script type="text/javascript">
    var aladin;
    A.init.then(() => {
        aladin = A.aladin('#aladin-lite-div', {fullScreen: true, cooFrame: "ICRSd", showSimbadPointerControl: true, showShareControl: true, showShareControl: true, survey: 'https://alasky.cds.unistra.fr/DSS/DSSColor/', fov: 180, showContextMenu: true});
    });
</script>

</body>
</html>

I hope this helps

szpetny commented 7 months ago

@bmatthieu3 Yes indeed it looks like I have skipped wasm-pack build command. Now it works. And the arrows also are in place. Thanks!

bmatthieu3 commented 7 months ago

Super! I suppose I can close the issue then. @ghansham you should have a couple of hints to do to your case which seems similar to @szpetny . If you still encounter troubles, please tell us in #129

ghansham commented 7 months ago

Okay. Good to know. Will try at my end at some point of time. Right now busy with some other activity.

On Wed, 6 Mar, 2024, 4:36 pm Matthieu Baumann, @.***> wrote:

Super! I suppose I can close the issue then. @ghansham https://github.com/ghansham you should have a couple of hints to do to your case which seems similar to @szpetny https://github.com/szpetny . If you still encounter troubles, please tell us in #129 https://github.com/cds-astro/aladin-lite/issues/129

— Reply to this email directly, view it on GitHub https://github.com/cds-astro/aladin-lite/issues/137#issuecomment-1980620246, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYXFJBPZG6R33FT76M2A6LYW32CNAVCNFSM6AAAAABEBY6KB2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBQGYZDAMRUGY . You are receiving this because you were mentioned.Message ID: @.***>