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

86121908 copy schedule #1371

Merged
merged 17 commits into from Feb 7, 2015
Merged

86121908 copy schedule #1371

merged 17 commits into from Feb 7, 2015

Conversation

macumber
Copy link
Contributor

No description provided.

@macumber
Copy link
Contributor Author

@asparke2 @DavidGoldwasser this is ready to review. Includes ability to copy schedules and the various schedule editor improvements we discussed. Only one I was not able to do was "When click on schedule show the default profile"

@asparke2
Copy link
Member

asparke2 commented Feb 5, 2015

@macumber Looks pretty good, a few minor things:

New schedule dialog:
1. Users will be confused about which "Temperature" or "Temperature 1" to pick. We should see what objects/fields use the discrete temperature and change them to accept the continuous temperature if E+ will allow it. If there must be two Temperature types, we should give them better names so users know which one to pick.
2. When you switch to SI units, the new schedule dialog still shows upper and lower limits in IP.
3. When a schedule is unitless, get rid of the "()" or change to "(unitless)" after the "numeric type" and "limits" fields.

Schedule UI:
1. "Enter Value" text is too small (maybe increase to same size as axis labels?)
2. Is it possible to show the value centered over every segment all the time? Then, when you hover over the segment, change the text to say "Enter Value" like it does now. This could help address Taylor's comments.

Other Minor Things (don't change unless very easy):
1. When you zoom in to 15 Minute view, the "Enter Value" text shows in middle of segment, which might be off the screen.
2. Still that goofy workflow where making a new rule forces you to hit "+" then "Add" even though there is only 1 radio button choice. Leave this if there are plans to add more radio button options (like selecting which rule to copy or something) in near future.

@macumber
Copy link
Contributor Author

macumber commented Feb 5, 2015

Thanks @asparke2, I'll get on it

I worry that putting the values over each segment would be too cluttered, I'll see if it is easy to do to try it out

I wasn't going to stretch to the copy rule button but maybe I will do that

@macumber
Copy link
Contributor Author

macumber commented Feb 5, 2015

Discrete temperature schedules are:

https://github.com/NREL/OpenStudio/blob/develop/openstudiocore/src/model/ScheduleTypeRegistry.cpp#L171
"CoilCoolingLowTempRadiantVarFlow","Cooling Control Temperature Schedule"

https://github.com/NREL/OpenStudio/blob/develop/openstudiocore/src/model/ScheduleTypeRegistry.cpp#L296
"ZoneHVACLowTemperatureRadiantElectric","Heating Setpoint Temperature"

https://github.com/NREL/OpenStudio/blob/develop/openstudiocore/src/model/ScheduleTypeRegistry.cpp#L311
"ZoneHVACHighTemperatureRadiant","Heating Setpoint Temperature"

I'm not sure why these would require a discrete temperature schedule (or what that would even mean). @asparke2 @kbenne @mbadams5 any ideas on the repercussions of making these continuous?

@macumber
Copy link
Contributor Author

macumber commented Feb 5, 2015

@asparke2 I allow users to use any user created schedule type limits in their model, I figure if they have put them there they would have given them good names and know what to look for

The alternative would be to only allow users to select the built in schedule types when creating a new schedule

@macumber
Copy link
Contributor Author

macumber commented Feb 5, 2015

@lefticus CppCheck is finding a memory error at this line https://nrel.github.io/OpenStudioBuildResults/OpenStudio-d64bacf14a2f37403310f227856763d6df969e67-x86_64-Linux-Ubuntu-14.04-cppcheck-1.61.html

https://nrel.github.io/OpenStudioBuildResults/OpenStudio-d64bacf14a2f37403310f227856763d6df969e67-x86_64-Linux-Ubuntu-14.04-cppcheck-1.61.html

I don't doubt that I might be doing this wrong but the way I read the realloc page is that you don't have to delete the pointer before calling realloc? Should I just call delete and then malloc to be safe?

http://www.cplusplus.com/reference/cstdlib/realloc/

…sure what the repercussions are. @asparke2 @kbenne @mbadams5 please review

"CoilCoolingLowTempRadiantVarFlow","Cooling Control Temperature Schedule"

"ZoneHVACLowTemperatureRadiantElectric","Heating Setpoint Temperature"

"ZoneHVACHighTemperatureRadiant","Heating Setpoint Temperature"
@macumber
Copy link
Contributor Author

macumber commented Feb 5, 2015

"Enter Value" text is too small (maybe increase to same size as axis labels?)

I don't quite understand what is going on here, I'm using a QGraphicsTextItem and have to set the font to something enormous for it to even be visible:

https://github.com/NREL/OpenStudio/blob/86121908_copy_schedule/openstudiocore/src/openstudio_lib/ScheduleDayView.cpp#L1519

Setting font in html doesn't seem to work. @axelstudios @kbenne @evanweaver have you ever used QGraphicsTextItem? Do you know how to get it to look right?

@kbenne
Copy link
Contributor

kbenne commented Feb 5, 2015

@macumber I have used graphics text. In the HVAC outliner for example.

What is going on here is that there is some extreme scaling tricks going in in the graphics view to enable zoom and scroll. I don't remember all of the details, but basically this is why I say I made a mistake when I used QGraphicsView to enable the zoom feature in the schedules view.

screen shot 2015-02-05 at 1 35 26 pm

@macumber
Copy link
Contributor Author

macumber commented Feb 5, 2015

@kbenne thanks, I think the solution will be to figure out the scaling and apply the inverse to the text in some way. What is your availability to look at this on this branch?

@lefticus
Copy link
Contributor

lefticus commented Feb 5, 2015

@macumber the problem is that realloc might return null, at which point you've lost the original pointer.

I believe they would have you do something like:

auto newptr = (LPBYTE)realloc(lpBuffer, dwBufferSize);
if (newptr == nullptr) {
  // whoops we had a memory allocation error and we need to clean things up
  free(lpBuffer);
  throw std::runtime_error("Memory error while attempting to allocate buffer")
} else {
  lpBuffer = newptr;
}

moreData = true;

This way you properly handle the error condition, not just leak memory if something with realloc goes wrong.

If you do encounter an error here, it's likely fatal, as the system probably ran out of RAM. So an even more abrupt exit() might even be called for.

@macumber
Copy link
Contributor Author

macumber commented Feb 6, 2015

@lefticus thanks for the tip, I implemented your solution

@macumber
Copy link
Contributor Author

macumber commented Feb 6, 2015

@asparke2 this is good for you to review again

asparke2 added a commit that referenced this pull request Feb 7, 2015
@asparke2 asparke2 merged commit cbf8a40 into develop Feb 7, 2015
@asparke2 asparke2 deleted the 86121908_copy_schedule branch February 7, 2015 18:19
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.

None yet

6 participants