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

When recording to a.bag file that is being installed in Program Files in defa… #8156

Merged
merged 5 commits into from
Jun 13, 2021

Conversation

manson
Copy link

@manson manson commented Jan 14, 2021

…ult read-only folder it fails. Now it attemts to write to temp folder first.

ev-mp
ev-mp previously requested changes Mar 24, 2021
Copy link
Collaborator

@ev-mp ev-mp left a comment

Choose a reason for hiding this comment

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

@manson , thank you for the contribution.
Can you try to rework the PR using the 3rd-party used in the SDK?

std::string tmp_path = std::getenv("TEMP");
std::string bag = "a.bag";

#ifdef _WIN32
Copy link
Collaborator

Choose a reason for hiding this comment

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

Avoid platform-specific #ifdefs and explicit query of environment vars.
Replace with existing 3rd-party from common/os.h

Then I believe, the rest will be unnecessary

Copy link
Collaborator

Choose a reason for hiding this comment

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

@manson, is there any update?
putting @maloel on notice

Copy link
Author

@manson manson Apr 13, 2021

Choose a reason for hiding this comment

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

@ev-mp
just pushed new commit, copying here description from commit:
rs-record-payback now uses existing universal function get_folder_path from common folder. Previously it was defined in /common/os.cpp file dealing with files functions, file dialogs and so on in one file, but direct usage of it led to a conflict with existing Opengl libraries used by examples (and rs-record-payback) as it loads this as well. So files functions (without dialogs) was extracted to a file fs.cpp (and fs.h) and this functions may be used in any part of realsense solution.os.h includes fs.h as well and any possible usage of this functions by including os.h leaves it operational (but with Opengl libraries included). rs-record-payback now directly includes fs module to get temporary system folder to record to a temporary file.

Andrey Uzbekov and others added 5 commits June 10, 2021 13:48
…ult read-only folder it fails. Now it attemts to write to temp folder first.
…older_path``` from ```common``` folder. Previously it was defined in ```/common/os.cpp``` file dealing with files functions, file dialogs and so on in one file, but direct usage of it led to a conflict with existing Opengl libraries used by examples (and ```rs-record-payback```) as it loads this as well. So files functions (without dialogs) was extracted to a file fs.cpp (and fs.h) and this functions may be used in any part of realsense solution.```os.h``` includes ```fs.h``` as well and any possible usage of this functions by including ```os.h``` leaves it operational (but with Opengl libraries included). ```rs-record-payback``` now directly includes ```fs``` module to get temporary system folder to record to a temporary file.
@maloel maloel force-pushed the fix_record-and-playback branch from 7274447 to b646e01 Compare June 10, 2021 11:50
@maloel maloel dismissed ev-mp’s stale review June 10, 2021 11:51

Not fast enough :)

set_property(TARGET rs-record-playback PROPERTY CXX_STANDARD 11)
target_link_libraries(rs-record-playback ${DEPENDENCIES})
include_directories(../ ../../third-party/tclap/include ../../third-party/imgui)
include_directories(../../common)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ev-mp
Will this create an issue with the examples project we install?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it will...

Copy link
Contributor

@maloel maloel left a comment

Choose a reason for hiding this comment

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

This may cause issues with the installed example project -- will be fixed in followup.

@maloel maloel merged commit 0bf3ce8 into IntelRealSense:development Jun 13, 2021
maloel added a commit that referenced this pull request Jun 13, 2021
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.

3 participants