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/3044/margin renders some buildings missing #3077

Merged
merged 47 commits into from Nov 22, 2022

Conversation

RomanenkoVladimir
Copy link
Member

@RomanenkoVladimir RomanenkoVladimir commented Oct 6, 2022

Modify area calculation to prevent disappearing buildings

Please read the CONTRIBUTING.md before opening a PR.

closes: #3044, #3096, #3081

Description

  • Change algorithm to visualize buildings that are getting removed by margin
  • Make margin scale with map size
  • Change floor label calculation to prevent floor labels that are "sandwitched" between bigger folders from rendering without buildings
  • add tests to explain new behaviour
  • maps are now quite bigger to assure that all buildings fit
  • sizes of folder areas should now more closely match each other when they are the same size

Screenshots or gifs

@RomanenkoVladimir
Copy link
Member Author

However, I am still unhappy with the floor label solution and what be glad for any feedback or the potential to discuss it

@RomanenkoVladimir
Copy link
Member Author

I would appreciate your feedback, now that maps are again squares and big maps are scaled down a bit

@RomanenkoVladimir
Copy link
Member Author

Also for the changed floor label criteria and overall applicability for presentations

@ce-bo
Copy link
Collaborator

ce-bo commented Nov 18, 2022

It is looking good to me. We should check one last thing.
With experimental features = on, the anon.cc.json is crashing with the following errors:
image

Copy link
Contributor

@MW-Friedrich MW-Friedrich left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks a lot for writing some documentation too, that helped a lot 👍

@sonarcloud
Copy link

sonarcloud bot commented Nov 22, 2022

[CodeCharta Analysis] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Nov 22, 2022

[CodeCharta Visualization] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

96.6% 96.6% Coverage
0.0% 0.0% Duplication

@MW-Friedrich MW-Friedrich merged commit b3b12e7 into main Nov 22, 2022
@MW-Friedrich MW-Friedrich deleted the bug/3044/margin-renders-some-buildings-missing branch November 22, 2022 14:50
@BridgeAR
Copy link
Member

BridgeAR commented Nov 22, 2022

Dear team, I would like to celebrate fixing this somehow. Do you have a suggestion what you would like to do or would you rather not do anything special?

@ce-bo
Copy link
Collaborator

ce-bo commented Nov 22, 2022

It is looking good to me. We should check one last thing. With experimental features = on, the anon.cc.json is crashing with the following errors: image

@MW-Friedrich Did you have a look on that?

@MW-Friedrich
Copy link
Contributor

It is looking good to me. We should check one last thing. With experimental features = on, the anon.cc.json is crashing with the following errors: image

@MW-Friedrich Did you have a look on that?

Yes, I did, and I unfortunately couldn't reproduce the error. The anon.cc.json is the file that is loaded when starting CodeCarta in development mode (npm run dev), right?

@ce-bo
Copy link
Collaborator

ce-bo commented Nov 23, 2022

Ah okay. No, the anon.cc.json is another file that is shared internally. I will provide you with the map.

@MW-Friedrich
Copy link
Contributor

Ah okay. No, the anon.cc.json is another file that is shared internally. I will provide you with the map.

I found and fixed the error together with @BenediktMehl in another issue, since we found out that the error had nothing to do with the margins. Good catch though!

@ce-bo
Copy link
Collaborator

ce-bo commented Nov 23, 2022

Ah okay. No, the anon.cc.json is another file that is shared internally. I will provide you with the map.

I found and fixed the error together with @BenediktMehl in another issue, since we found out that the error had nothing to do with the margins. Good catch though!

Thank you. This was fixed in #3144

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

Successfully merging this pull request may close these issues.

Margin renders some buildings invisible
4 participants