Commit 943ec6fe authored by Laszlo Agocs's avatar Laszlo Agocs

Once again change how swapchain sizing works

So the surface/layer reported size gets priority over what
we can calculate from the QWindow. On some platforms the values
can differ...
parent 82a3c250
......@@ -194,7 +194,7 @@ void Window::customRender()
u->updateDynamicBuffer(d.ubuf, 0, 64, mvp.constData());
QRhiCommandBuffer *cb = m_sc->currentFrameCommandBuffer();
const QSize outputSizeInPixels = m_sc->effectivePixelSize();
const QSize outputSizeInPixels = m_sc->currentPixelSize();
cb->beginPass(m_sc->currentFrameRenderTarget(), { 0.4f, 0.7f, 0.0f, 1.0f }, { 1.0f, 0 }, u);
......
......@@ -214,7 +214,7 @@ void Window::customRender()
u->updateDynamicBuffer(d.ubuf, 0, 64, mvp.constData());
QRhiCommandBuffer *cb = m_sc->currentFrameCommandBuffer();
const QSize outputSizeInPixels = m_sc->effectivePixelSize();
const QSize outputSizeInPixels = m_sc->currentPixelSize();
cb->beginPass(m_sc->currentFrameRenderTarget(), { 0.4f, 0.7f, 0.0f, 1.0f }, { 1.0f, 0 }, u);
......
......@@ -370,20 +370,18 @@ void Window::releaseResources()
void Window::resizeSwapChain()
{
const QSize outputSize = size() * devicePixelRatio();
const QSize outputSize = m_sc->surfacePixelSize();
m_ds->setPixelSize(outputSize);
m_ds->build(); // == m_ds->release(); m_ds->build();
m_sc->setRequestedPixelSize(outputSize);
m_hasSwapChain = m_sc->buildOrResize();
m_elapsedMs = 0;
m_elapsedCount = 0;
const QSize outputSizeInPixels = m_sc->effectivePixelSize();
m_proj = m_r->clipSpaceCorrMatrix();
m_proj.perspective(45.0f, outputSizeInPixels.width() / (float) outputSizeInPixels.height(), 0.01f, 100.0f);
m_proj.perspective(45.0f, outputSize.width() / (float) outputSize.height(), 0.01f, 100.0f);
m_proj.translate(0, 0, -4);
}
......@@ -403,7 +401,7 @@ void Window::render()
// If the window got resized or got newly exposed, resize the swapchain.
// (the newly-exposed case is not actually required by some
// platforms/backends, but f.ex. Vulkan on Windows seems to need it)
if (m_sc->requestedPixelSize() != size() * devicePixelRatio() || m_newlyExposed) {
if (m_sc->currentPixelSize() != m_sc->surfacePixelSize() || m_newlyExposed) {
resizeSwapChain();
if (!m_hasSwapChain)
return;
......@@ -454,7 +452,7 @@ void Window::render()
u->updateDynamicBuffer(m_ubuf, 64, 4, &m_opacity);
QRhiCommandBuffer *cb = m_sc->currentFrameCommandBuffer();
const QSize outputSizeInPixels = m_sc->effectivePixelSize();
const QSize outputSizeInPixels = m_sc->currentPixelSize();
// Apply buffer updates, clear, start the renderpass (where applicable).
cb->beginPass(m_sc->currentFrameRenderTarget(), { 0.4f, 0.7f, 0.0f, 1.0f }, { 1.0f, 0 }, u);
......
......@@ -242,7 +242,7 @@ void Window::customRender()
cb->endPass(u);
// onscreen (quad)
const QSize outputSizeInPixels = m_sc->effectivePixelSize();
const QSize outputSizeInPixels = m_sc->currentPixelSize();
cb->beginPass(m_sc->currentFrameRenderTarget(), { 0.4f, 0.7f, 0.0f, 1.0f }, { 1.0f, 0 });
cb->setGraphicsPipeline(d.ps);
cb->setViewport({ 0, 0, float(outputSizeInPixels.width()), float(outputSizeInPixels.height()) });
......
......@@ -314,7 +314,7 @@ void Window::customRender()
cb->endPass();
// onscreen
const QSize outputSizeInPixels = m_sc->effectivePixelSize();
const QSize outputSizeInPixels = m_sc->currentPixelSize();
cb->beginPass(m_sc->currentFrameRenderTarget(), { 0.4f, 0.7f, 0.0f, 1.0f }, { 1.0f, 0 });
cb->setGraphicsPipeline(d.psLeft); // showing the non-msaa version
cb->setViewport({ 0, 0, float(outputSizeInPixels.width()), float(outputSizeInPixels.height()) });
......
......@@ -321,20 +321,18 @@ void Window::releaseResources()
void Window::resizeSwapChain()
{
const QSize outputSize = size() * devicePixelRatio();
const QSize outputSize = m_sc->surfacePixelSize();
m_ds->setPixelSize(outputSize);
m_ds->build(); // == m_ds->release(); m_ds->build();
m_sc->setRequestedPixelSize(outputSize);
m_hasSwapChain = m_sc->buildOrResize();
m_elapsedMs = 0;
m_elapsedCount = 0;
const QSize outputSizeInPixels = m_sc->effectivePixelSize();
m_proj = m_r->clipSpaceCorrMatrix();
m_proj.perspective(45.0f, outputSizeInPixels.width() / (float) outputSizeInPixels.height(), 0.01f, 100.0f);
m_proj.perspective(45.0f, outputSize.width() / (float) outputSize.height(), 0.01f, 100.0f);
m_proj.translate(0, 0, -4);
}
......@@ -354,7 +352,7 @@ void Window::render()
// If the window got resized or got newly exposed, resize the swapchain.
// (the newly-exposed case is not actually required by some
// platforms/backends, but f.ex. Vulkan on Windows seems to need it)
if (m_sc->requestedPixelSize() != size() * devicePixelRatio() || m_newlyExposed) {
if (m_sc->currentPixelSize() != m_sc->surfacePixelSize() || m_newlyExposed) {
resizeSwapChain();
if (!m_hasSwapChain)
return;
......
......@@ -179,12 +179,11 @@ void ExampleWindow::releaseResources()
void ExampleWindow::recreateSwapChain()
{
const QSize outputSize = size() * devicePixelRatio();
const QSize outputSize = m_sc->surfacePixelSize();
m_ds->setPixelSize(outputSize);
m_ds->build(); // == m_ds->release(); m_ds->build();
m_sc->setRequestedPixelSize(outputSize);
m_hasSwapChain = m_sc->buildOrResize();
m_resizedSwapChain = true;
}
......@@ -202,7 +201,7 @@ void ExampleWindow::render()
if (!m_hasSwapChain || m_notExposed)
return;
if (m_sc->requestedPixelSize() != size() * devicePixelRatio() || m_newlyExposed) {
if (m_sc->currentPixelSize() != m_sc->surfacePixelSize() || m_newlyExposed) {
recreateSwapChain();
if (!m_hasSwapChain)
return;
......@@ -221,7 +220,7 @@ void ExampleWindow::render()
return;
}
const QSize outputSize = m_sc->effectivePixelSize();
const QSize outputSize = m_sc->currentPixelSize();
if (m_resizedSwapChain) {
m_resizedSwapChain = false;
m_triRenderer.resize(outputSize);
......
......@@ -272,7 +272,7 @@ void Window::customRender()
}
QRhiCommandBuffer *cb = m_sc->currentFrameCommandBuffer();
const QSize outputSizeInPixels = m_sc->effectivePixelSize();
const QSize outputSizeInPixels = m_sc->currentPixelSize();
cb->beginPass(m_sc->currentFrameRenderTarget(), { 0.4f, 0.7f, 0.0f, 1.0f }, { 1.0f, 0 }, u);
......
......@@ -85,7 +85,7 @@ void Renderer::initSwapChainResources()
{
m_sc->buildOrResize(); // this just wraps the qvulkanwindow's swapchain and other resources
m_triRenderer.resize(m_sc->effectivePixelSize());
m_triRenderer.resize(m_sc->currentPixelSize());
}
void Renderer::releaseSwapChainResources()
......@@ -118,7 +118,7 @@ void Renderer::startNextFrame()
m_triRenderer.queueResourceUpdates(u);
cb->beginPass(m_sc->currentFrameRenderTarget(), { 0.4f, 0.7f, 0.0f, 1.0f }, { 1.0f, 0 }, u);
m_triRenderer.queueDraw(cb, m_sc->effectivePixelSize());
m_triRenderer.queueDraw(cb, m_sc->currentPixelSize());
cb->endPass();
m_r->endFrame(m_sc);
......
......@@ -850,9 +850,6 @@ public:
QWindow *window() const { return m_window; }
void setWindow(QWindow *window) { m_window = window; }
QSize requestedPixelSize() const { return m_requestedPixelSize; }
void setRequestedPixelSize(const QSize &size) { m_requestedPixelSize = size; }
SurfaceImportFlags flags() const { return m_flags; }
void setFlags(SurfaceImportFlags f) { m_flags = f; }
......@@ -870,21 +867,27 @@ public:
QObject *target() const { return m_target; }
void setTarget(QObject *obj) { m_target = obj; }
// Returns the size with which the swapchain was last successfully built.
// Use this to decide or buildOrResize() needs to be called again: if
// currentPixelSize() != surfacePixelSize() then the swapchain needs to
// be resized.
QSize currentPixelSize() const { return m_currentPixelSize; }
virtual QRhiCommandBuffer *currentFrameCommandBuffer() = 0;
virtual QRhiRenderTarget *currentFrameRenderTarget() = 0;
// Applications are expected to use requestedPixelSize() for logic like "if
// qwindow->size() * qwindow->dpr() != requestedPixelSize() then
// resize_swapchain", and effectivePixelSize() for all graphics
// calculations, like the viewport. On some platforms they may not be the
// same, e.g. some Vulkan implementations on Windows were observed to make
// the swapchain buffers' height off by one on high dpi screens, this is
// then reflected in the effective size.
virtual QSize effectivePixelSize() const = 0;
// To be called before build() with relevant parameters like depthStencil and sampleCount set.
// As an exception to the common rules, m_depthStencil is not required to be built yet.
// Note setRenderPassDescriptor(), that must still be called afterwards (but before buildOrResize()).
// The size of the window's associated surface or layer. Do not assume this
// is the same as size() * devicePixelRatio()! Can be called before
// buildOrResize() (with window set), which allows setting the correct size
// for the depth-stencil buffer that is then used together with the
// swapchain's color buffers. Also used in combination with
// currentPixelSize() to detect size changes.
virtual QSize surfacePixelSize() = 0;
// To be called before build() with relevant parameters like depthStencil
// and sampleCount set. As an exception to the common rules, m_depthStencil
// is not required to be built yet. Note setRenderPassDescriptor(), that
// must still be called afterwards (but before buildOrResize()).
virtual QRhiRenderPassDescriptor *newCompatibleRenderPassDescriptor() = 0;
// As the name suggests, this is slightly different from the typical
......@@ -898,12 +901,12 @@ public:
protected:
QRhiSwapChain(QRhiImplementation *rhi);
QWindow *m_window = nullptr;
QSize m_requestedPixelSize;
SurfaceImportFlags m_flags;
QRhiRenderBuffer *m_depthStencil = nullptr;
int m_sampleCount = 1;
QRhiRenderPassDescriptor *m_renderPassDesc = nullptr;
QObject *m_target = nullptr;
QSize m_currentPixelSize;
void *m_reserved;
};
......
......@@ -2422,9 +2422,10 @@ QRhiRenderTarget *QD3D11SwapChain::currentFrameRenderTarget()
return &rt;
}
QSize QD3D11SwapChain::effectivePixelSize() const
QSize QD3D11SwapChain::surfacePixelSize()
{
return pixelSize;
Q_ASSERT(m_window);
return m_window->size() * m_window->devicePixelRatio();
}
QRhiRenderPassDescriptor *QD3D11SwapChain::newCompatibleRenderPassDescriptor()
......@@ -2477,7 +2478,8 @@ bool QD3D11SwapChain::buildOrResize()
Q_ASSERT(!swapChain || window == m_window);
window = m_window;
pixelSize = m_requestedPixelSize;
m_currentPixelSize = surfacePixelSize();
pixelSize = m_currentPixelSize;
colorFormat = DXGI_FORMAT_R8G8B8A8_UNORM;
const DXGI_FORMAT srgbAdjustedFormat = m_flags.testFlag(sRGB) ?
......
......@@ -386,7 +386,7 @@ struct QD3D11SwapChain : public QRhiSwapChain
QRhiCommandBuffer *currentFrameCommandBuffer() override;
QRhiRenderTarget *currentFrameRenderTarget() override;
QSize effectivePixelSize() const override;
QSize surfacePixelSize() override;
QRhiRenderPassDescriptor *newCompatibleRenderPassDescriptor() override;
bool buildOrResize();
......
......@@ -2042,9 +2042,10 @@ QRhiRenderTarget *QGles2SwapChain::currentFrameRenderTarget()
return &rt;
}
QSize QGles2SwapChain::effectivePixelSize() const
QSize QGles2SwapChain::surfacePixelSize()
{
return pixelSize;
Q_ASSERT(m_window);
return m_window->size() * m_window->devicePixelRatio();
}
QRhiRenderPassDescriptor *QGles2SwapChain::newCompatibleRenderPassDescriptor()
......@@ -2055,7 +2056,8 @@ QRhiRenderPassDescriptor *QGles2SwapChain::newCompatibleRenderPassDescriptor()
bool QGles2SwapChain::buildOrResize()
{
surface = m_window;
pixelSize = m_requestedPixelSize;
m_currentPixelSize = surfacePixelSize();
pixelSize = m_currentPixelSize;
rt.d.rp = QRHI_RES(QGles2RenderPassDescriptor, m_renderPassDesc);
rt.d.pixelSize = pixelSize;
......
......@@ -412,7 +412,7 @@ struct QGles2SwapChain : public QRhiSwapChain
QRhiCommandBuffer *currentFrameCommandBuffer() override;
QRhiRenderTarget *currentFrameRenderTarget() override;
QSize effectivePixelSize() const override;
QSize surfacePixelSize() override;
QRhiRenderPassDescriptor *newCompatibleRenderPassDescriptor() override;
bool buildOrResize() override;
......
......@@ -1889,7 +1889,7 @@ QRhiRenderTarget *QMetalSwapChain::currentFrameRenderTarget()
return &rtWrapper;
}
QSize QMetalSwapChain::effectivePixelSize() const
QSize QMetalSwapChain::surfacePixelSize()
{
return pixelSize;
}
......
......@@ -222,7 +222,7 @@ struct QMetalSwapChain : public QRhiSwapChain
QRhiCommandBuffer *currentFrameCommandBuffer() override;
QRhiRenderTarget *currentFrameRenderTarget() override;
QSize effectivePixelSize() const override;
QSize surfacePixelSize() override;
QRhiRenderPassDescriptor *newCompatibleRenderPassDescriptor() override;
......
......@@ -858,10 +858,10 @@ bool QRhiVulkan::createOffscreenRenderPass(VkRenderPass *rp,
return true;
}
bool QRhiVulkan::recreateSwapChain(VkSurfaceKHR surface, const QSize &pixelSize,
QRhiSwapChain::SurfaceImportFlags flags, QRhiSwapChain *swapChain)
bool QRhiVulkan::recreateSwapChain(QRhiSwapChain *swapChain)
{
if (pixelSize.isEmpty())
QVkSwapChain *swapChainD = QRHI_RES(QVkSwapChain, swapChain);
if (swapChainD->pixelSize.isEmpty())
return false;
df->vkDeviceWaitIdle(dev);
......@@ -878,25 +878,12 @@ bool QRhiVulkan::recreateSwapChain(VkSurfaceKHR surface, const QSize &pixelSize,
}
}
VkSurfaceCapabilitiesKHR surfaceCaps;
vkGetPhysicalDeviceSurfaceCapabilitiesKHR(physDev, surface, &surfaceCaps);
vkGetPhysicalDeviceSurfaceCapabilitiesKHR(physDev, swapChainD->surface, &surfaceCaps);
quint32 reqBufferCount = QVkSwapChain::DEFAULT_BUFFER_COUNT;
if (surfaceCaps.maxImageCount)
reqBufferCount = qBound(surfaceCaps.minImageCount, reqBufferCount, surfaceCaps.maxImageCount);
QVkSwapChain *swapChainD = QRHI_RES(QVkSwapChain, swapChain);
VkExtent2D bufferSize = surfaceCaps.currentExtent;
if (bufferSize.width == quint32(-1)) {
Q_ASSERT(bufferSize.height == quint32(-1));
bufferSize.width = swapChainD->m_requestedPixelSize.width();
bufferSize.height = swapChainD->m_requestedPixelSize.height();
}
swapChainD->pixelSize = QSize(bufferSize.width, bufferSize.height);
if (swapChainD->pixelSize.isEmpty())
return false;
VkSurfaceTransformFlagBitsKHR preTransform =
(surfaceCaps.supportedTransforms & VK_SURFACE_TRANSFORM_IDENTITY_BIT_KHR)
? VK_SURFACE_TRANSFORM_IDENTITY_BIT_KHR
......@@ -907,13 +894,13 @@ bool QRhiVulkan::recreateSwapChain(VkSurfaceKHR surface, const QSize &pixelSize,
? VK_COMPOSITE_ALPHA_INHERIT_BIT_KHR
: VK_COMPOSITE_ALPHA_OPAQUE_BIT_KHR;
if (flags.testFlag(QRhiSwapChain::SurfaceHasPreMulAlpha)
if (swapChainD->m_flags.testFlag(QRhiSwapChain::SurfaceHasPreMulAlpha)
&& (surfaceCaps.supportedCompositeAlpha & VK_COMPOSITE_ALPHA_PRE_MULTIPLIED_BIT_KHR))
{
compositeAlpha = VK_COMPOSITE_ALPHA_PRE_MULTIPLIED_BIT_KHR;
}
if (flags.testFlag(QRhiSwapChain::SurfaceHasNonPreMulAlpha)
if (swapChainD->m_flags.testFlag(QRhiSwapChain::SurfaceHasNonPreMulAlpha)
&& (surfaceCaps.supportedCompositeAlpha & VK_COMPOSITE_ALPHA_POST_MULTIPLIED_BIT_KHR))
{
compositeAlpha = VK_COMPOSITE_ALPHA_POST_MULTIPLIED_BIT_KHR;
......@@ -924,17 +911,18 @@ bool QRhiVulkan::recreateSwapChain(VkSurfaceKHR surface, const QSize &pixelSize,
if (swapChainD->supportsReadback)
usage |= VK_IMAGE_USAGE_TRANSFER_SRC_BIT;
qDebug("Creating new swapchain of %d buffers, size %dx%d", reqBufferCount, bufferSize.width, bufferSize.height);
qDebug("Creating new swapchain of %d buffers, size %dx%d",
reqBufferCount, swapChainD->pixelSize.width(), swapChainD->pixelSize.height());
VkSwapchainKHR oldSwapChain = swapChainD->sc;
VkSwapchainCreateInfoKHR swapChainInfo;
memset(&swapChainInfo, 0, sizeof(swapChainInfo));
swapChainInfo.sType = VK_STRUCTURE_TYPE_SWAPCHAIN_CREATE_INFO_KHR;
swapChainInfo.surface = surface;
swapChainInfo.surface = swapChainD->surface;
swapChainInfo.minImageCount = reqBufferCount;
swapChainInfo.imageFormat = swapChainD->colorFormat;
swapChainInfo.imageColorSpace = swapChainD->colorSpace;
swapChainInfo.imageExtent = bufferSize;
swapChainInfo.imageExtent = VkExtent2D { uint32_t(swapChainD->pixelSize.width()), uint32_t(swapChainD->pixelSize.height()) };
swapChainInfo.imageArrayLayers = 1;
swapChainInfo.imageUsage = usage;
swapChainInfo.imageSharingMode = VK_SHARING_MODE_EXCLUSIVE;
......@@ -1157,7 +1145,7 @@ QRhi::FrameOpResult QRhiVulkan::beginWrapperFrame(QRhiSwapChain *swapChain)
swapChainD->cbWrapper.cb = w->currentCommandBuffer();
swapChainD->rtWrapper.d.fb = w->currentFramebuffer();
swapChainD->m_requestedPixelSize = swapChainD->pixelSize = swapChainD->rtWrapper.d.pixelSize = w->swapChainImageSize();
swapChainD->m_currentPixelSize = swapChainD->pixelSize = swapChainD->rtWrapper.d.pixelSize = w->swapChainImageSize();
currentFrameSlot = w->currentFrame();
currentSwapChain = swapChainD;
......@@ -4060,9 +4048,27 @@ QRhiRenderTarget *QVkSwapChain::currentFrameRenderTarget()
return &rtWrapper;
}
QSize QVkSwapChain::effectivePixelSize() const
QSize QVkSwapChain::surfacePixelSize()
{
return pixelSize;
if (m_target) {
QVulkanWindow *vkw = qobject_cast<QVulkanWindow *>(m_target);
return vkw ? vkw->swapChainImageSize() : QSize();
}
ensureSurface();
// The size from the QWindow may not exactly match the surface... so if a
// size is reported from the surface, use that.
VkSurfaceCapabilitiesKHR surfaceCaps;
memset(&surfaceCaps, 0, sizeof(surfaceCaps));
QRHI_RES_RHI(QRhiVulkan);
rhiD->vkGetPhysicalDeviceSurfaceCapabilitiesKHR(rhiD->physDev, surface, &surfaceCaps);
VkExtent2D bufferSize = surfaceCaps.currentExtent;
if (bufferSize.width == quint32(-1)) {
Q_ASSERT(bufferSize.height == quint32(-1));
return m_window->size() * m_window->devicePixelRatio();
}
return QSize(bufferSize.width, bufferSize.height);
}
QRhiRenderPassDescriptor *QVkSwapChain::newCompatibleRenderPassDescriptor()
......@@ -4100,6 +4106,7 @@ QRhiRenderPassDescriptor *QVkSwapChain::newCompatibleRenderPassDescriptor()
bool QVkSwapChain::ensureSurface()
{
Q_ASSERT(m_window);
surface = QVulkanInstance::surfaceForWindow(m_window);
if (!surface) {
qWarning("Failed to get surface for window");
......@@ -4155,7 +4162,7 @@ bool QVkSwapChain::buildOrResize()
if (vkw) {
rtWrapper.d.rp = QRHI_RES(QVkRenderPassDescriptor, m_renderPassDesc);
Q_ASSERT(rtWrapper.d.rp && rtWrapper.d.rp->rp);
m_requestedPixelSize = pixelSize = rtWrapper.d.pixelSize = vkw->swapChainImageSize();
m_currentPixelSize = pixelSize = rtWrapper.d.pixelSize = vkw->swapChainImageSize();
rtWrapper.d.colorAttCount = 1;
rtWrapper.d.dsAttCount = 1;
rtWrapper.d.msaaAttCount = vkw->sampleCountFlagBits() > VK_SAMPLE_COUNT_1_BIT ? 1 : 0;
......@@ -4172,8 +4179,11 @@ bool QVkSwapChain::buildOrResize()
if (!ensureSurface())
return false;
m_currentPixelSize = surfacePixelSize();
pixelSize = m_currentPixelSize;
QRHI_RES_RHI(QRhiVulkan);
if (!rhiD->recreateSwapChain(surface, m_requestedPixelSize, m_flags, this))
if (!rhiD->recreateSwapChain(this))
return false;
if (m_depthStencil && m_depthStencil->sampleCount() != m_sampleCount) {
......
......@@ -257,7 +257,7 @@ struct QVkSwapChain : public QRhiSwapChain
QRhiCommandBuffer *currentFrameCommandBuffer() override;
QRhiRenderTarget *currentFrameRenderTarget() override;
QSize effectivePixelSize() const override;
QSize surfacePixelSize() override;
QRhiRenderPassDescriptor *newCompatibleRenderPassDescriptor() override;
bool buildOrResize() override;
......@@ -387,8 +387,7 @@ public:
VkImageAspectFlags aspectMask, VkSampleCountFlagBits samples,
VkDeviceMemory *mem, VkImage *images, VkImageView *views, int count);
bool recreateSwapChain(VkSurfaceKHR surface, const QSize &pixelSize, QRhiSwapChain::SurfaceImportFlags flags,
QRhiSwapChain *swapChain);
bool recreateSwapChain(QRhiSwapChain *swapChain);
void releaseSwapChainResources(QRhiSwapChain *swapChain);
VkFormat optimalDepthStencilFormat();
......
......@@ -61,6 +61,7 @@ dxc for d3d as an alternative to fxc?
hlsl -> dxc -> spirv -> spirv-cross hmmm...
+++ done
clear window vs surface/layer size mess
mtl: msaa (onscreen)
mtl: buffer upload with offset/size
mtl: texcopy
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment