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

3.0.1-beta1: GbXMLReverseTranslator applies incorrect scaling to windows #3997

Closed
jmarrec opened this issue Jun 16, 2020 · 3 comments · Fixed by #3998
Closed

3.0.1-beta1: GbXMLReverseTranslator applies incorrect scaling to windows #3997

jmarrec opened this issue Jun 16, 2020 · 3 comments · Fixed by #3998

Comments

@jmarrec
Copy link
Collaborator

jmarrec commented Jun 16, 2020

Issue overview

With lengthUnit="Feet", the windows are incorrectly placed.

gbxml scaling issue

Reported by @antonszilasi

Current Behavior

Wrong scaling for windows

Expected Behavior

Windows should be placed correctly, like they are in 2.9.1.

Steps to Reproduce

I will make a unit test for this.

Possible Solution

Move from QXml to pugixml missed a conversion factor m_lengthMutiplier as it previously existed here: https://github.com/NREL/OpenStudio/blame/e65085e23f2792b8726bdd6f3f143c31651d136a/openstudiocore/src/gbxml/ReverseTranslator.cpp#L559

Details

Environment

Some additional details about your environment for this issue (if relevant):

  • Platform (Operating system, version): All
  • Version of OpenStudio (if using an intermediate build, include SHA): 3.0.1-beta1, c710464
@antonszilasi
Copy link

antonszilasi commented Jun 16, 2020

@jmarrec thanks for taking the initiative to fix this issue but im confused why this wasnt fixed here:
#3951 (comment)
@tijcolem
would it be possible to apply this fix to openstudio:3.0.1-rc1 as well?

@jmarrec
Copy link
Collaborator Author

jmarrec commented Jun 17, 2020

@antonszilasi #3951 is a different error, specific to surface Type assignment, and was a typo in if blocks.

This one is about scaling and is about a missing multiplication.

I can speculate the original file you sent for 3951 may have had the same problem, but I don't recall it having windows. Anyways I didn't identify a problem with scaling there.
Note that I also have set my diff editor (vim) to ignore vertices since they often suffer from rounding errors especially due to sketchup, so when diffing between 2.9.1 OSM and 3.x OSM I saw the surface types problems only.

Perhaps that's where the confusion comes from?

This is a good illustration of why writing good, detailed issues matters: the issue templates we have should help structure that by listing all the things you think are not working correctly to maximize the chances of seeing them addressed.
I personally don't use gbxml (or perhaps twice a year), so these aren't bugs I already know of.

@tijcolem
Copy link
Collaborator

@antonszilasi Yes, but it'll be called openstudio:3.0.1-rc2 with this change.

tijcolem added a commit that referenced this issue Jun 18, 2020
Fix #3997 - GbXMLReverseTranslator applies incorrect scaling to windows when unit isn't meter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants