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

VYZN: Matrix Messaging Integration #455

Merged
merged 25 commits into from Jan 21, 2023

Conversation

clecherbauer
Copy link
Contributor

@clecherbauer clecherbauer commented Oct 26, 2022

This pull request implements the lib "matrix-widget-api" which allows us to send control-messages to an embedded (iframe) bldrs instance.

Messages bldrs can receive:

  • ai.bldrs-share.LoadModel (bldrs loads a ifc-model from github)
  • ai.bldrs-share.SelectElements (bldrs select elements from the current model in the viewer)

Messages bldrs will emit:

  • ai.bldrs-share.ElementsSelected (triggers if elements are added to the selection)
  • ai.bldrs-share.ElementsDeSelected (triggers if elements are removed from the selection)

@netlify
Copy link

netlify bot commented Oct 26, 2022

Deploy Preview for bldrs-share ready!

Name Link
🔨 Latest commit 55ec16a
🔍 Latest deploy log https://app.netlify.com/sites/bldrs-share/deploys/63cbfdb6fcca7e0008e35daf
😎 Deploy Preview https://deploy-preview-455--bldrs-share.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@pablo-mayrgundter pablo-mayrgundter marked this pull request as draft October 26, 2022 14:33
@oo-bldrs
Copy link
Contributor

The title of this PR should be changed to reflect the contents, which if I'm guessing correctly is: Matrix Messaging Integration?

@clecherbauer clecherbauer changed the title Bldrs to vyzn Matrix Messaging Integration Oct 31, 2022
@oo-bldrs oo-bldrs marked this pull request as ready for review October 31, 2022 15:57
Copy link
Contributor

@oo-bldrs oo-bldrs left a comment

Choose a reason for hiding this comment

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

I've added some comments regarding what's present so far.

src/WidgetApi/ApiConnection.js Outdated Show resolved Hide resolved
src/index.jsx Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@pablo-mayrgundter pablo-mayrgundter marked this pull request as draft November 1, 2022 12:10
Copy link
Member

@pablo-mayrgundter pablo-mayrgundter left a comment

Choose a reason for hiding this comment

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

Thanks, nice start!

Let's leave this in draft until we get tests and styles. Left you some initial commements. I'll help with the global init when we can chat.

src/WidgetApi/ApiConnection.js Outdated Show resolved Hide resolved
src/index.jsx Outdated Show resolved Hide resolved
src/WidgetApi/WidgetApi.js Outdated Show resolved Hide resolved
src/WidgetApi/WidgetApi.js Outdated Show resolved Hide resolved
src/store/useStore.js Outdated Show resolved Hide resolved
@pablo-mayrgundter
Copy link
Member

Here's our style guide:

https://github.com/bldrs-ai/Share/wiki/Dev:-Style

Please use our eslint config or run yarn lint manually

@pablo-mayrgundter
Copy link
Member

Per discussion, I'd try useMemo or useCallback for the init logic. Assuming that works, the right place is probably in src/Share.jsx.

@pablo-mayrgundter
Copy link
Member

Hey Christian, how's this going?

@clecherbauer
Copy link
Contributor Author

@pablo-mayrgundter i still need to write tests. Could you do another review now before is start with that?

Copy link
Member

@pablo-mayrgundter pablo-mayrgundter left a comment

Choose a reason for hiding this comment

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

Basically looking good.

Would you please update the description of the PR to summarize the changes and a link to the staging instance you're testing with.

src/WidgetApi/WidgetApi.js Outdated Show resolved Hide resolved
src/WidgetApi/WidgetApi.js Show resolved Hide resolved
src/WidgetApi/ApiEventsRegistry.js Outdated Show resolved Hide resolved
Adrian62D and others added 2 commits December 16, 2022 16:45
@clecherbauer clecherbauer marked this pull request as ready for review December 19, 2022 08:46
@pablo-mayrgundter
Copy link
Member

Hmm, I'm not seeing the netlify dialog change. can you try updating smth like that yarn.lock and see if that triggers it?

@clecherbauer
Copy link
Contributor Author

OK, i did update yarn.lock. But does not seem like it triggered the deployment. We really need to have the current version of this code deployed since the viewer in our production is loaded from this preview-instance.

@oo-bldrs
Copy link
Contributor

oo-bldrs commented Jan 6, 2023

I manually flushed the build cache and retried the build which appeared to have deployed successfully.

Does the deploy preview reflect your changes?

https://deploy-preview-455--bldrs-share.netlify.app/

@clecherbauer
Copy link
Contributor Author

yes, thank you

Ibrahim5aad added a commit to Ibrahim5aad/Share that referenced this pull request Jan 9, 2023
@Adrian62D
Copy link

This PR will be replaced by #525.

This means, once #525 is merged and once the vyzn systems have adapted to the new URL, #455 can be abandoned.

@OlegMoshkovich OlegMoshkovich changed the title Matrix Messaging Integration VYZN: Matrix Messaging Integration Jan 10, 2023
@Adrian62D Adrian62D self-assigned this Jan 10, 2023
@pablo-mayrgundter
Copy link
Member

Hey Adrian, I'm reluctant to abandon this PR.. we had a bunch of comments in flight. Can we continue here?

@Adrian62D
Copy link

Hi Pablo, going directly for #525 would be easier. However, if you wish you can first merge this #455. But we should not invest more time #455.

@pablo-mayrgundter
Copy link
Member

Per discussion with Adrian yesterday, Christian, please do finish this out. I think we're good to go but pending a successful merge and final review (hopefully a no-op).

@clecherbauer
Copy link
Contributor Author

@pablo-mayrgundter Hi Pablo, @aozien has rebased and fixed my changes. I think we're ready for your final review now :)

@pablo-mayrgundter
Copy link
Member

Looks like we got broken somehow :(

I tried loading a model and got this error. I see some maybe related changes in the last update.

image

@aozien
Copy link
Collaborator

aozien commented Jan 20, 2023

@pablo-mayrgundter It turns out the problem was caused by the (yarn.lock) file re-generated in commit (a8ed57d), since that did some minor-version updates to some packages, which caused some functionality to break.
The yarn lock file is now changed to match exactly the one on share/main for the already existing packages.

@pablo-mayrgundter
Copy link
Member

Works for me now, thanks.

Found another issue.. permalinks for camera and cutplane are broken :(

https://deploy-preview-455--bldrs-share.netlify.app/share/v/p/index.ifc/89/112/139/154/621#c:43.83,54.18,47.56,-47,15,-5.73::p:x=0

When I follow this link, everything after the index.ifc is removed after load. Same on a loaded model.

Please fix that and add an integration test for it.

Thank you!

@aozien
Copy link
Collaborator

aozien commented Jan 20, 2023

@pablo-mayrgundter this should be fixed now as well :)

Abdel Rahman Osama and others added 2 commits January 21, 2023 16:49
Signed-off-by: Pablo Mayrgundter <pablo.mayrgundter@gmail.com>
@pablo-mayrgundter pablo-mayrgundter merged commit d131471 into bldrs-ai:main Jan 21, 2023
@pablo-mayrgundter
Copy link
Member

Finally! Congrats guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

None yet

5 participants