Skip to content

Deflect 1.0#195

Merged
rdumusc merged 4 commits intoBlueBrain:masterfrom
rdumusc:master
May 4, 2018
Merged

Deflect 1.0#195
rdumusc merged 4 commits intoBlueBrain:masterfrom
rdumusc:master

Conversation

@rdumusc
Copy link

@rdumusc rdumusc commented May 2, 2018

No description provided.

Raphael Dumusc added 2 commits May 2, 2018 16:13
All the server-specific classes are now in the deflect::server
namespace.
@rdumusc rdumusc requested a review from dnachbaur May 2, 2018 14:21
@rdumusc rdumusc force-pushed the master branch 2 times, most recently from 7811b88 to 95cfe8c Compare May 2, 2018 15:34
//@{
uint32_t width = 0u; /**< The width in pixels. */
uint32_t height = 0u; /**< The height in pixels. */
//@}
Copy link
Contributor

Choose a reason for hiding this comment

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

those four and the data type is a repetition from the segment parameters. reuse them here?

Copy link
Author

Choose a reason for hiding this comment

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

This is a tricky point. The existing structs Segment and SegementParameters are shared between Deflect and DeflectServer because they are part of the networking protocol. However, they were also part of the public DeflectServer API because they were exposed through the Frame object. The problem was that DeflectServer depends on Deflect, so the shared classes should logically be in Deflect, but the headers have no reason to be public in Deflect because they are not used by client code. In the end I decided to keep the legacy structs private in Deflect and introduce this new Tile struct for use in Frame in DeflectServer that combines the two. It's nicer to use without the nested SegementParameters object and the public API can evolve separately from the networking API, at the cost of some duplication.

deflect/types.h Outdated
/** The possible formats for image data. */
enum class Format : std::uint8_t
{
rgba = 0, // equivalent to old compressed=false property
Copy link
Contributor

Choose a reason for hiding this comment

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

with 1.0, maybe those references to old things are no longer needed to be mentioned

Copy link
Author

Choose a reason for hiding this comment

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

You're right, it probably doesn't serve a purpose anymore. Anyone changing this enum will break the network protocol, but that's true of all the other enums as well, so no reason to mark those two as "special".

)

set(DEFLECTSERVER_LINK_LIBRARIES
PUBLIC Deflect Qt5::Core Qt5::Network
Copy link
Contributor

Choose a reason for hiding this comment

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

network is public?

Copy link
Author

Choose a reason for hiding this comment

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

Because Server : public QTcpServer. It probably wouldn't be too hard to hide it in the future.

deflect/types.h Outdated

using Segments = std::vector<Segment>;

namespace server
Copy link
Contributor

Choose a reason for hiding this comment

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

move them to a separate server/types.h?

Copy link
Author

Choose a reason for hiding this comment

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

OK, will do, also for qt/types.h then.

* [194](https://github.com/BlueBrain/Deflect/pull/194):
Reset API versionning to 1.0, remove deprecated functions (Stream::asyncSend,
ImageWrapper::swapYAxis).
ImageWrapper::swapYAxis). Moved all server-specific classes to a separate
Copy link
Contributor

Choose a reason for hiding this comment

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

that change is not part of 194?!


cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
project(Deflect VERSION 0.14.1)
project(Deflect VERSION 1.0.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah!

Copy link
Author

Choose a reason for hiding this comment

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

:-)

@@ -1,200 +0,0 @@
/*********************************************************************/
Copy link
Contributor

Choose a reason for hiding this comment

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

? haven't seen this file ever? wow

Copy link
Author

Choose a reason for hiding this comment

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

I found it by accident, it had been forgotten since 2013...!

doc/Changelog.md Outdated

### 1.0.0 (git master)
* [194](https://github.com/BlueBrain/Deflect/pull/194):
Reset API versionning to 1.0, remove deprecated functions (Stream::asyncSend,
Copy link
Contributor

Choose a reason for hiding this comment

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

one 'n' too much in versioning

Copy link
Author

Choose a reason for hiding this comment

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

thanks, done

@rdumusc
Copy link
Author

rdumusc commented May 3, 2018

Damn, two unit tests just failed in "closeAndReopenStreamWithObserverStayingAliveAndRendering" with a failed assert(!frame->tiles.empty()); but I can't reproduce it locally, must be a nasty timing issue...

@rdumusc
Copy link
Author

rdumusc commented May 3, 2018

retest this please

1 similar comment
@rdumusc
Copy link
Author

rdumusc commented May 3, 2018

retest this please

@rdumusc rdumusc merged commit bbde77a into BlueBrain:master May 4, 2018
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.

2 participants