View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0031956 | Open CASCADE | OCCT:Visualization | public | 2020-11-24 10:04 | 2023-04-14 14:58 |
Reporter | kgv | Assigned To | rodrlyra | ||
Priority | normal | Severity | feature | ||
Status | resolved | Resolution | open | ||
Product Version | 7.4.0 | ||||
Target Version | 7.8.0 | ||||
Summary | 0031956: Visualization - provide Image_AlienPixMap::Save() writing into a memory buffer instead of a file | ||||
Description | Image_AlienPixMap::Load() provides overloads reading image from file path, memory buffer and std::istream. In contrast, Image_AlienPixMap::Save() is currently able to write only into the given file path, which restricts class usability. | ||||
Steps To Reproduce | test v3d/bugs/bug31956 | ||||
Tags | No tags attached. | ||||
Test case number | v3d bugs bug31956 | ||||
|
Branch CR31956 has been created by CheskoArt. SHA-1: fbf48c08439946f24061d1b7d2f6b0bb78ed656a Detailed log of new commits: Author: achesnok Date: Mon Sep 6 14:36:40 2021 +0300 0031956: Visualization - provide Image_AlienPixMap::Save() writing into a memory buffer instead of a file Added two new Image_AlienPixMap::Save() overloads, taking std::ostream or memory buffer arguments. |
|
Branch CR31956 has been updated forcibly by CheskoArt. SHA-1: 58887fde1aeca0408c1dc7f872b53386626370a0 |
|
Branch CR31956 has been updated forcibly by CheskoArt. SHA-1: c5594d863afbdf330ae4dae912be6ca5133d74b2 |
|
Please review the patch. Test results: http://jenkins-test-occt.nnov.opencascade.com/view/CR31956-master-achesnok/view/ALL/ |
|
+#ifdef HAVE_FREEIMAGE + Standard_EXPORT FIBITMAP* GetImageToDump (int theFormat); FIBITMAP* myLibImage; This breaks binary compatibility and should be reverted. HAVE_FREEIMAGE should not occur in .hxx header. |
|
Branch CR31956 has been updated forcibly by CheskoArt. SHA-1: 5303e6b270b86fbcd8a769b5fab4fa2e653c0604 |
|
Branch CR31956 has been updated forcibly by CheskoArt. SHA-1: db9b3fca3d8c457239a50b13e33c5ae5bf9dc868 |
|
Patch fixed |
|
Review request. |
|
-: myLibImage (NULL) +#ifdef HAVE_FREEIMAGE + : myLibImage (NULL) +#endif Won't this break ''Image_AlienPixMap::Clear''? This feature needs validation for all build types. + //! when theData isn't NULL used only to determine format What's "theData"? + //! @param theBuffer buffer pointer where to write What are the NULL buffer semantics here? + Standard_EXPORT bool Save (const TCollection_AsciiString& theFileName, + Standard_Byte* theBuffer = NULL, + Standard_Size theLength = 0); Of all the Save/Load functions here, this is the only one that does not take filename as the last argument. Let's not sacrifice API consistency to avoid adding one more function. |
|
Branch CR31956_1 has been created by mzernova. SHA-1: db9b3fca3d8c457239a50b13e33c5ae5bf9dc868 No new revisions were added by this update. |
|
Branch CR31956 has been updated forcibly by mzernova. SHA-1: 2307f70538bb6f13358f6e14f8cdc3ad63277f00 |
|
Branch CR31956 has been updated forcibly by mzernova. SHA-1: b49c83d4330962d7a20c3d0dc0f27f7b6c33b843 |
|
Branch CR31956_1 has been updated forcibly by mzernova. SHA-1: f327716896c5dddfb5c22d8f370598c16ace25dd |
|
http://jenkins-test-occt.nnov.opencascade.com/view/CR31956_1-master-mzernova/view/ALL/ |
|
OCCT branch: CR31956_1 |
|
Deat mzernova Please update branch and retest fix |
|
Branch CR31956_1 has been updated forcibly by mzernova. SHA-1: 189127fc4a95cb163317d2faeb865c1ce73d370b |
|
Branch CR31956_1 has been updated forcibly by mzernova. SHA-1: fad361b45ee627dc645fc8aa5c27dcf87d4dc60c |
|
Dear rodrlyra, please review OCCT - CR31956_1 Tests: http://jenkins-test-10.nnov.opencascade.com/view/CR31956-master-mzernova/view/COMPARE/ (Some tests fail due to jenkins issues.) New test case was created. |
|
Dear @mzernova There a few style remarks, please check (they are just recommendations) I think commens is not fully. You implement two methods that working with Path and stream, if i understand correctle Added two new Image_AlienPixMap::Save() overloads, taking std::ostream or memory buffer arguments. I think we use a little differ doxygen style @param[in] @param[out] + //! Write image data to file or memory buffer using file extension to determine format. + //! @param theBuffer buffer pointer where to write + //! when NULL, function write image data to theFileName file + //! @param theLength memory buffer length + //! @param theFileName file name to save; + //! when theBuffer isn't NULL used only to determine format Not changable parameters should be const + Standard_EXPORT bool Save (Standard_Byte* theBuffer, + Standard_Size theLength, + const TCollection_AsciiString& theFileName); Private methods should not be STANDARD_EXPORT. not changable parameter shoud be const. Something wrong with spaces separating Private: and methods and fields. Best practices are using privete fields and protected methods. +private: + Standard_EXPORT FIBITMAP* GetImageToDump (int theFormat); FIBITMAP* myLibImage; What is it? Why You do not use Messager::SendFaile or theDI << There more cases then I can explane below + if (ViewerTest::CurrentView().IsNull()) + { + std::cout << "Error: no active view\n"; + return 1; + } ... + else + { + std::cout << "Syntax error at '" << anArg << "'\n"; + return 1; + } ... + if (aFile.get() == NULL) + { + std::cout << "Syntax error: image file '" << anImgPath << "' cannot be found\n"; + return 1; + } |
|
Dear @mzernova, taking from Dmitry remarks, I would reinforce some corrections I think is necessary: 1 - Change '@param' from the documentation to '@param[in]' if it is input parameter, or '@param[out]' if output. 2 - Delete the 'Standard_EXPORT' from private method. 3 - On QABugs_1, the use of std::cout indeed does not seem correct. Taking the example of other QABugs, the output should be written to the 'Draw_Interpretor' parameter. Lastly, I have seen that the new test you created tests two overloads of the Save() function: using stream, and using buffer. I think it is also important to add a test to the overload that saves to a file path. |
|
Branch CR31956_1 has been updated forcibly by mzernova. SHA-1: 3209a916243e50bcd7d7146e044d4e2a45332656 |
|
Branch CR31956_1 has been updated by mzernova. SHA-1: 4f1308d94e2e11fbde5930bb716294261284597a Detailed log of new commits: Author: mzernova Date: Wed Apr 5 16:37:52 2023 +0100 # remarks |
|
Branch CR31956_1 has been updated forcibly by mzernova. SHA-1: b961a59c42ee9f834b30d9c5a407cae75e78413c |
|
Dear rodrlyra, please review OCCT - CR31956_1 Tests: http://jenkins-test-10.nnov.opencascade.com/view/CR31956-master-mzernova/view/COMPARE/ (Version for Windows and some tests fail due to jenkins issues.) Also I don't think that we need an additional test to save the image to a file path. This is old functionality and is used to dump images in many tests. |
Date Modified | Username | Field | Change |
---|---|---|---|
2020-11-24 10:04 | kgv | New Issue | |
2020-11-24 10:04 | kgv | Assigned To | => kgv |
2020-11-24 10:06 | kgv | Relationship added | related to 0030182 |
2020-11-24 10:06 | kgv | Relationship replaced | child of 0030182 |
2020-11-24 10:06 | kgv | Product Version | => 7.4.0 |
2020-11-24 10:06 | kgv | Relationship added | related to 0030549 |
2021-08-11 11:31 |
|
Assigned To | kgv => CheskoArt |
2021-08-11 11:53 |
|
Status | new => assigned |
2021-09-14 14:12 | git | Note Added: 0104074 | |
2021-09-15 21:34 | git | Note Added: 0104104 | |
2021-09-15 22:04 | git | Note Added: 0104105 | |
2021-09-16 19:07 |
|
Note Added: 0104131 | |
2021-09-16 19:07 |
|
Assigned To | CheskoArt => kgv |
2021-09-16 19:07 |
|
Status | assigned => resolved |
2021-09-16 19:07 |
|
Steps to Reproduce Updated | |
2021-09-16 19:23 | kgv | Note Added: 0104132 | |
2021-09-16 21:05 | git | Note Added: 0104134 | |
2021-09-18 08:04 |
|
Assigned To | kgv => CheskoArt |
2021-09-18 08:04 |
|
Status | resolved => assigned |
2021-09-18 14:36 | git | Note Added: 0104215 | |
2021-09-18 17:37 |
|
Note Added: 0104217 | |
2021-09-18 17:37 |
|
Assigned To | CheskoArt => kgv |
2021-09-18 17:37 |
|
Status | assigned => resolved |
2021-09-18 17:37 |
|
Steps to Reproduce Updated | |
2021-09-19 10:01 | kgv | Target Version | 7.6.0 => 7.7.0 |
2022-10-04 16:52 |
|
Assigned To | kgv => vpozdyayev |
2022-10-04 16:52 |
|
Note Added: 0111388 | |
2022-10-10 11:35 |
|
Note Added: 0111449 | |
2022-10-10 11:35 |
|
Note Edited: 0111449 | |
2022-10-10 11:36 |
|
Note Edited: 0111449 | |
2022-10-10 11:36 |
|
Note Edited: 0111449 | |
2022-10-10 11:38 |
|
Note Edited: 0111449 | |
2022-10-10 11:39 |
|
Note Edited: 0111449 | |
2022-10-10 15:54 |
|
Assigned To | vpozdyayev => mzernova |
2022-10-10 15:56 |
|
Status | resolved => assigned |
2022-10-24 10:43 |
|
Target Version | 7.7.0 => 7.8.0 |
2022-11-17 10:16 | git | Note Added: 0112141 | |
2022-11-17 10:52 | git | Note Added: 0112143 | |
2022-11-17 11:02 | git | Note Added: 0112144 | |
2022-11-17 11:02 | git | Note Added: 0112145 | |
2022-11-18 06:21 | mzernova | Note Added: 0112161 | |
2022-11-18 06:22 | mzernova | Note Added: 0112162 | |
2022-11-18 06:23 | mzernova | Assigned To | mzernova => vpozdyayev |
2022-11-18 06:23 | mzernova | Status | assigned => resolved |
2022-11-18 06:23 | mzernova | Steps to Reproduce Updated | |
2023-01-24 19:21 | eugeny.belousov_162739 | Assigned To | vpozdyayev => bugmaster |
2023-02-09 14:24 | dpasukhi | Assigned To | bugmaster => mzernova |
2023-02-09 14:24 | dpasukhi | Status | resolved => assigned |
2023-02-09 14:24 | dpasukhi | Note Added: 0113099 | |
2023-02-10 13:32 | git | Note Added: 0113106 | |
2023-02-15 15:26 | git | Note Added: 0113148 | |
2023-02-15 15:36 | mzernova | Assigned To | mzernova => rodrlyra |
2023-02-15 15:36 | mzernova | Status | assigned => resolved |
2023-02-15 15:36 | mzernova | Note Added: 0113149 | |
2023-02-15 15:36 | mzernova | Test case number | => v3d/bugs/bug31956 |
2023-02-15 15:37 | mzernova | Test case number | v3d/bugs/bug31956 => v3d bugs bug31956 |
2023-02-15 16:06 | dpasukhi | Note Added: 0113150 | |
2023-02-27 18:48 | rodrlyra | Note Added: 0113211 | |
2023-02-27 18:49 | rodrlyra | Assigned To | rodrlyra => mzernova |
2023-02-27 18:49 | rodrlyra | Status | resolved => assigned |
2023-04-05 10:48 | git | Note Added: 0113362 | |
2023-04-05 18:38 | git | Note Added: 0113367 | |
2023-04-05 18:50 | git | Note Added: 0113368 | |
2023-04-14 14:58 | mzernova | Assigned To | mzernova => rodrlyra |
2023-04-14 14:58 | mzernova | Status | assigned => resolved |
2023-04-14 14:58 | mzernova | Note Added: 0113383 | |
2023-04-14 14:58 | mzernova | Note Edited: 0113383 |