#4077 Remove name field from common HVAC data objects that are not visible to user #4081
Conversation
jmarrec
added
component - HVAC
component - Model
Enhancement Request
Pull Request - Ready for CI
| } else { | ||
| std::string n = name(); | ||
| if (n == "OS:PortList") { | ||
| result.push_back("ConnectionObject"); | ||
| result.push_back("PortLists"); | ||
| } else if (n == "OS:Connection") { | ||
| result.push_back("ConnectionNames"); | ||
| } |
There was a problem hiding this comment.
Hardcoded references in IddObject instead of on the name field in the OpenStudio.idd
| if ((candidate.iddObject().type() == openstudio::IddObjectType::OS_Connection) || | ||
| (candidate.iddObject().type() == openstudio::IddObjectType::OS_PortList)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
First workaround I had to do that I didn't anticipate. WorkspaceObject_Impl::uniquelyIdentifiableByName would crash at OS_ASSERT(candidateName)
| if (auto s = oTarget->name()) { | ||
| return s.get(); | ||
| } else { | ||
| // Fall back to handle if it doesn't have a name | ||
| return openstudio::toString(oTarget->handle()); | ||
| } |
There was a problem hiding this comment.
Second workaround I had to do that I didn't anticipate. IdfExtensibleGroup::fields() would call this one and crash because it doesn't have a name.
src/osversion/VersionTranslator.cpp
Outdated
| } else if ((iddname == "OS:Connection") || (iddname == "OS:PortList")) { | ||
| // Deleted the 'Name' field | ||
| auto iddObject = idd_3_1_0.getObject(iddname); | ||
| IdfObject newObject(iddObject.get()); | ||
| for (size_t i = 0; i < object.numFields(); ++i) { | ||
| if ((value = object.getString(i))) { | ||
| if (i < 1) { | ||
| newObject.setString(i, value.get()); | ||
| } else if ( i == 1 ) { | ||
| // Deleted the name: no-op | ||
| } else { | ||
| newObject.setString(i-1, value.get()); | ||
| } | ||
| } | ||
| } | ||
| m_refactored.push_back(RefactoredObjectData(object, newObject)); | ||
| ss << newObject; | ||
|
|
There was a problem hiding this comment.
VersionTranslation, straightforward
resources/model/OpenStudio.idd
Outdated
| A2, \field Name | ||
| \type alpha | ||
| \reference ConnectionNames | ||
| A3, \field Source Object |
There was a problem hiding this comment.
Removed name field in IDD
|
@jmarrec what would you like to do with this? If think the changes are good, but I guess we didn't get the performance bump we were hoping for. Still I think if the conflicts are resolved I think this could be merged. |
Conflicts: src/model/Connection.cpp src/model/PortList.cpp src/osversion/VersionTranslator.cpp
|
@kbenne I resolved conflicts. Let's see if CI has a problem with it or not. |
|
sorry. @jmarrec can you resolve conflicts again? |
|
@kbenne Resolvee, from github on web, so hopefully I didn't mess it up. Edit: Diff Looks fine I think it should be ok. |
|
CI Results for 3ea567c:
|
Pull request overview
First pass as #4077 for some benchmarking and co.
Please see timings on #4077 directly, but here are the sum by test type / timing type:
So this is not exactly faster. My implementation can probably be improved though.
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
src/model/test)src/energyplus/Test)src/osversion/VersionTranslator.cpp)src/openstudio_lib/library/OpenStudioPolicy.xml)Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.